Issue 29555782: Issue 5701 - Inconsistent forum links and DNT link (Closed)
|
Created:
Sept. 25, 2017, 3:30 p.m. by saroyanm Modified:
Sept. 25, 2017, 9:45 p.m. Visibility:
Public. |
DescriptionIssue 5701 - Inconsistent forum links and DNT link
The DNT redirection in the infrastructure side is under the review -> https://codereview.adblockplus.org/29548672/ But that shouldn't be blocker for this review.
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed Sebastian's comments #
Total comments: 4
Patch Set 3 : Fallback #
Total comments: 4
Patch Set 4 : Revert Patch set #3 #Patch Set 5 : Use placeholder for application #
Total comments: 5
Patch Set 6 : #
MessagesTotal messages: 15
@Thomas can you please have a look on this review please ? https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js#newc... new-options.js:872: { I wonder if we need to check for the "gecko" and assign Firefox to it ? Same way we assign chrome to all chromium platforms that are not "opera" ? Considering the fact that application for the Edge browser is "edge" we shouldn't have problem here I guess.
https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js#newc... new-options.js:872: { On 2017/09/25 15:59:50, saroyanm wrote: > I wonder if we need to check for the "gecko" and assign Firefox to it ? > Same way we assign chrome to all chromium platforms that are not "opera" ? That is an excellent idea. Though in practice it wouldn't be a problem right now, as we don't show this link on Firefox Mobile, and there are no other Gecko applications that support WebExtensions. But in case this changes in the future, I think it would be a good idea to get the logic here right from the beginning. > Considering the fact that application for the Edge browser is "edge" we > shouldn't have problem here I guess. Yes, application=edge is hard-coded in the Microsoft Edge builds, so no need to explicitly handle this here.
https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js#newc... new-options.js:870: Promise.all([getPromisedInfo("platform"), getPromisedInfo("application")]) Perhaps it would be simpler to move this logic into messageResponder.js, so that you don't have to send two messages in the first place.
Thanks for having look into this review Sebastian. New patch is uploaded. https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js#newc... new-options.js:870: Promise.all([getPromisedInfo("platform"), getPromisedInfo("application")]) On 2017/09/25 16:12:48, Sebastian Noack wrote: > Perhaps it would be simpler to move this logic into messageResponder.js, so that > you don't have to send two messages in the first place. I agree that sending two messages is not a good idea. Done. https://codereview.adblockplus.org/29555782/diff/29555783/new-options.js#newc... new-options.js:872: { On 2017/09/25 16:05:55, Sebastian Noack wrote: > On 2017/09/25 15:59:50, saroyanm wrote: > > I wonder if we need to check for the "gecko" and assign Firefox to it ? > > Same way we assign chrome to all chromium platforms that are not "opera" ? > > That is an excellent idea. Though in practice it wouldn't be a problem right > now, as we don't show this link on Firefox Mobile, and there are no other Gecko > applications that support WebExtensions. But in case this changes in the future, > I think it would be a good idea to get the logic here right from the beginning. > > > Considering the fact that application for the Edge browser is "edge" we > > shouldn't have problem here I guess. > > Yes, application=edge is hard-coded in the Microsoft Edge builds, so no need to > explicitly handle this here. Done.
https://codereview.adblockplus.org/29555782/diff/29555819/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29555782/diff/29555819/new-options.js#newc... new-options.js:878: else if (platform == "gecko") Hmm I think in general we need a fallback for the non "gecko", "chromium" and "edge" browsers ? I think something along the lines: if (platform == "chromium" && application == "opera") application = "opera"; else if (platform == "gecko") application = "firefox"; else if (application == "edge") application = "edge"; else application = "chrome";
New patch uploaded. https://codereview.adblockplus.org/29555782/diff/29555819/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29555782/diff/29555819/new-options.js#newc... new-options.js:878: else if (platform == "gecko") On 2017/09/25 16:44:46, saroyanm wrote: > Hmm I think in general we need a fallback for the non "gecko", "chromium" and > "edge" browsers ? > > I think something along the lines: > > if (platform == "chromium" && application == "opera") > application = "opera"; > else if (platform == "gecko") > application = "firefox"; > else if (application == "edge") > application = "edge"; > else > application = "chrome"; Done.
https://codereview.adblockplus.org/29555782/diff/29555819/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29555782/diff/29555819/new-options.js#newc... new-options.js:878: else if (platform == "gecko") On 2017/09/25 16:44:46, saroyanm wrote: > Hmm I think in general we need a fallback for the non "gecko", "chromium" and > "edge" browsers ? > > I think something along the lines: > > if (platform == "chromium" && application == "opera") > application = "opera"; > else if (platform == "gecko") > application = "firefox"; > else if (application == "edge") > application = "edge"; > else > application = "chrome"; info.platform is hard-coded in the respective builds to "chromium", "gecko" and "edgehtml". As mentioned before, in the builds for Microsoft Edge, application is hard-coded as well. If we add further platforms in the future, we are going to create sub-forums and/or adapt the logic here as needed. IMO, there is no need to overthink the logic here, for now. In any case falling to the Chrome sub-forum would be the wrong place to send users of new platforms, anyway. https://codereview.adblockplus.org/29555782/diff/29555823/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29555782/diff/29555823/messageResponder.js... messageResponder.js:182: if (message.what == "browserInfo") Just an idea; how about merging the logic into the app.what[doclink] message: if (message.what == "doclink") { let application = info.application; if (info.platform == "chromium" && application != "opera") application = "chrome"; else if (info.platform == "gecko") application = "firefox; return Utils.getDocLink(message.link.replace("{application}", application); } That way we wouldn't have to introduce a new message sub-type, and the logic would be reusable.
https://codereview.adblockplus.org/29555782/diff/29555819/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29555782/diff/29555819/new-options.js#newc... new-options.js:878: else if (platform == "gecko") On 2017/09/25 17:09:22, Sebastian Noack wrote: > On 2017/09/25 16:44:46, saroyanm wrote: > > Hmm I think in general we need a fallback for the non "gecko", "chromium" and > > "edge" browsers ? > > > > I think something along the lines: > > > > if (platform == "chromium" && application == "opera") > > application = "opera"; > > else if (platform == "gecko") > > application = "firefox"; > > else if (application == "edge") > > application = "edge"; > > else > > application = "chrome"; > > info.platform is hard-coded in the respective builds to "chromium", "gecko" and > "edgehtml". As mentioned before, in the builds for Microsoft Edge, application > is hard-coded as well. If we add further platforms in the future, we are going > to create sub-forums and/or adapt the logic here as needed. IMO, there is no > need to overthink the logic here, for now. In any case falling to the Chrome > sub-forum would be the wrong place to send users of new platforms, anyway. Thanks for the info, I didn't know that we were hardcoding everywhere, I agree. reverted the "Fallback" patch. https://codereview.adblockplus.org/29555782/diff/29555823/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29555782/diff/29555823/messageResponder.js... messageResponder.js:182: if (message.what == "browserInfo") On 2017/09/25 17:09:22, Sebastian Noack wrote: > Just an idea; how about merging the logic into the app.what[doclink] message: > > if (message.what == "doclink") > { > let application = info.application; > if (info.platform == "chromium" && application != "opera") > application = "chrome"; > else if (info.platform == "gecko") > application = "firefox; > return Utils.getDocLink(message.link.replace("{application}", application); > } > > That way we wouldn't have to introduce a new message sub-type, and the logic > would be reusable. Don't have strong opinion, on one hand it makes this specific code reusable, but on other hand less flexible. I think having platform + application information in future will help us make more browser specific tweaks, like show browser specific messages. So we probably will introduce "browserInfo" if the issue is sending 1 request instead of 2.
https://codereview.adblockplus.org/29555782/diff/29555823/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29555782/diff/29555823/messageResponder.js... messageResponder.js:182: if (message.what == "browserInfo") On 2017/09/25 18:07:53, saroyanm wrote: > On 2017/09/25 17:09:22, Sebastian Noack wrote: > > Just an idea; how about merging the logic into the app.what[doclink] message: > > > > if (message.what == "doclink") > > { > > let application = info.application; > > if (info.platform == "chromium" && application != "opera") > > application = "chrome"; > > else if (info.platform == "gecko") > > application = "firefox; > > return Utils.getDocLink(message.link.replace("{application}", > application); > > } > > > > That way we wouldn't have to introduce a new message sub-type, and the logic > > would be reusable. > > Don't have strong opinion, on one hand it makes this specific code reusable, but > on other hand less flexible. > I think having platform + application information in future will help us make > more browser specific tweaks, like show browser specific messages. So we > probably will introduce "browserInfo" if the issue is sending 1 request instead > of 2. Also note that the reason you introduced "browserInfo" was to avoid an unnecessary round-trip to the background page. If you handle this logic in "doclink", you would avoid yet another round-trip. As for flexibility, I don't see why it matters much whether to implement it in the background page or on the content side, you are in control of both, and can just adapt the logic (or move it back to the content) whenever this becomes necessary. However, handling it in the "doclink" message seems to yield the simplest solution for now.
https://codereview.adblockplus.org/29555782/diff/29555823/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29555782/diff/29555823/messageResponder.js... messageResponder.js:182: if (message.what == "browserInfo") On 2017/09/25 19:07:47, Sebastian Noack wrote: > On 2017/09/25 18:07:53, saroyanm wrote: > > On 2017/09/25 17:09:22, Sebastian Noack wrote: > > > Just an idea; how about merging the logic into the app.what[doclink] > message: > > > > > > if (message.what == "doclink") > > > { > > > let application = info.application; > > > if (info.platform == "chromium" && application != "opera") > > > application = "chrome"; > > > else if (info.platform == "gecko") > > > application = "firefox; > > > return Utils.getDocLink(message.link.replace("{application}", > > application); > > > } > > > > > > That way we wouldn't have to introduce a new message sub-type, and the logic > > > would be reusable. > > > > Don't have strong opinion, on one hand it makes this specific code reusable, > but > > on other hand less flexible. > > I think having platform + application information in future will help us make > > more browser specific tweaks, like show browser specific messages. So we > > probably will introduce "browserInfo" if the issue is sending 1 request > instead > > of 2. > > Also note that the reason you introduced "browserInfo" was to avoid an > unnecessary round-trip to the background page. If you handle this logic in > "doclink", you would avoid yet another round-trip. As for flexibility, I don't > see why it matters much whether to implement it in the background page or on the > content side, you are in control of both, and can just adapt the logic (or move > it back to the content) whenever this becomes necessary. However, handling it in > the "doclink" message seems to yield the simplest solution for now. I agree with you, looks much more better now. Done.
https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js... messageResponder.js:160: let {application, platform} = info; Nit: I personally wouldn't assign "platform" to a local variable. That way you merely save typing "info" twice, but have to type "platform" one more time. Also IMO it is slightly less obvious that we are only modifying application in the logic here. But it might just be personal preference, so feel free to ignore. https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js... messageResponder.js:167: return Utils.getDocLink(link); Nit: Why a temporary variable, and not just return in the line above?
https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js... messageResponder.js:167: return Utils.getDocLink(link); On 2017/09/25 21:03:22, Sebastian Noack wrote: > Nit: Why a temporary variable, and not just return in the line above? Exceeded 80 char limit.
https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js... messageResponder.js:160: let {application, platform} = info; On 2017/09/25 21:03:22, Sebastian Noack wrote: > Nit: I personally wouldn't assign "platform" to a local variable. That way you > merely save typing "info" twice, but have to type "platform" one more time. Also > IMO it is slightly less obvious that we are only modifying application in the > logic here. But it might just be personal preference, so feel free to ignore. Done.
https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29555782/diff/29555857/messageResponder.js... messageResponder.js:167: return Utils.getDocLink(link); On 2017/09/25 21:17:33, saroyanm wrote: > On 2017/09/25 21:03:22, Sebastian Noack wrote: > > Nit: Why a temporary variable, and not just return in the line above? > > Exceeded 80 char limit. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
