Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507023003:
[NTP] Implement reactions to snippet context menu actions. (Closed)
Description[NTP] Implement reactions to snippet context menu actions.
Previously a lot of context menu options were not implemented.
This CL implements them.
BUG=664147, 663378, 664152
Committed: https://crrev.com/6699cc3c380b285d52ed7ce08b63738795876d92
Cr-Commit-Position: refs/heads/master@{#432902}
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase + open recent tabs properly. #
Total comments: 3
Patch Set 3 : treib@ comment. #
Total comments: 5
Patch Set 4 : dgn@ comment. #Patch Set 5 : bauerb@ comment. #
Total comments: 13
Patch Set 6 : bauerb@ comments. #Messages
Total messages: 48 (27 generated)
vitaliii@chromium.org changed reviewers: + bauerb@chromium.org
Hi Bernhard, could you redirect to someone from UI?
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:363: DownloadUtils.openFile( Would it make sense to move all the code paths that don't end up calling openUrl() to the front, and then funnel everything through a single openUrl() call?
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vitaliii@chromium.org changed reviewers: + treib@chromium.org
+ Marc for the metric change (see the comment). Hi Bernhard, I addressed your comment, please check. https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:363: DownloadUtils.openFile( On 2016/11/16 17:14:37, Bernhard Bauer wrote: > Would it make sense to move all the code paths that don't end up calling > openUrl() to the front, and then funnel everything through a single openUrl() > call? No. Currently there are multiple constraints and priorities here and adding another one (a single OpenUrl()) seems like not worth it. First, I am not sure how to do it without changing the behavior. Second, this function is already quite complex, I would rather vote for not combining cases in future when we add more categories (e.g. switch per windowOpenDisposition and inside handle each category explicitly). https://codereview.chromium.org/2507023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:382: if (article.mCategory == KnownCategories.ARTICLES Marc, could you have a look here? I believe that we do not want to track Bookmarks here. Also I assume that tracking opening offline version of an article is fine. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2507023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:382: if (article.mCategory == KnownCategories.ARTICLES On 2016/11/17 08:31:19, vitaliii wrote: > Marc, could you have a look here? > I believe that we do not want to track Bookmarks here. > Also I assume that tracking opening offline version of an article is fine. > WDYT? We do want to track all categories here (we even have a separate histogram for each category). Tracking an offline version should be fine, yes, as long as it's in the current tab.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey, I addressed Mark's comment (who may ignore this), everyone else - would you mind having a look? https://codereview.chromium.org/2507023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:382: if (article.mCategory == KnownCategories.ARTICLES On 2016/11/17 09:23:57, Marc Treib wrote: > On 2016/11/17 08:31:19, vitaliii wrote: > > Marc, could you have a look here? > > I believe that we do not want to track Bookmarks here. > > Also I assume that tracking opening offline version of an article is fine. > > WDYT? > > We do want to track all categories here (we even have a separate histogram for > each category). > > Tracking an offline version should be fine, yes, as long as it's in the current > tab. Done.
dgn@chromium.org changed reviewers: + dgn@chromium.org
Do we need to think about new UMA? We track which snippets are open with the window disposition, but we don't track things like whether it was offline, or if it's a downloaded asset, right? https://codereview.chromium.org/2507023003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:364: if (article.isDownload() && article.mIsDownloadedAsset) { as said on some other review, mIsDownloadedAssert implies isDownload() already. https://codereview.chromium.org/2507023003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:365: assert windowOpenDisposition == WindowOpenDisposition.CURRENT_TAB we don't propagate the windowDisposition here, why not assert only CURRENT_TAB and disable the other menu entries?
Hi dgn@, I addressed your comments. Please have a look. Regarding UMAs I would prefer this to be considered in a different CL. https://codereview.chromium.org/2507023003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:364: if (article.isDownload() && article.mIsDownloadedAsset) { On 2016/11/17 10:34:05, dgn wrote: > as said on some other review, mIsDownloadedAssert implies isDownload() already. Done. Sorry, for missing this here. https://codereview.chromium.org/2507023003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:365: assert windowOpenDisposition == WindowOpenDisposition.CURRENT_TAB On 2016/11/17 10:34:05, dgn wrote: > we don't propagate the windowDisposition here, why not assert only CURRENT_TAB > and disable the other menu entries? Patrick's decision. We want to show "open in new tab" and "new window" for offline page downloads and to be consistent we have to do this for assets, which you see above.
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2507023003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:365: assert windowOpenDisposition == WindowOpenDisposition.CURRENT_TAB On 2016/11/17 11:01:00, vitaliii wrote: > On 2016/11/17 10:34:05, dgn wrote: > > we don't propagate the windowDisposition here, why not assert only CURRENT_TAB > > and disable the other menu entries? > > Patrick's decision. > We want to show "open in new tab" and "new window" for offline page downloads > and to be consistent we have to do this for assets, which you see above. Acknowledged.
treib@ - feel free to ignore. So I addressed all the comments, could someone either add more comments or LGTM?
https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:363: DownloadUtils.openFile( On 2016/11/17 08:31:19, vitaliii wrote: > On 2016/11/16 17:14:37, Bernhard Bauer wrote: > > Would it make sense to move all the code paths that don't end up calling > > openUrl() to the front, and then funnel everything through a single openUrl() > > call? > > No. > Currently there are multiple constraints and priorities here and adding another > one (a single OpenUrl()) seems like not worth it. First, I am not sure how to do > it without changing the behavior. Second, this function is already quite > complex, I would rather vote for not combining cases in future when we add more > categories (e.g. switch per windowOpenDisposition and inside handle each > category explicitly). On 2016/11/17 08:31:19, vitaliii wrote: > On 2016/11/16 17:14:37, Bernhard Bauer wrote: > > Would it make sense to move all the code paths that don't end up calling > > openUrl() to the front, and then funnel everything through a single openUrl() > > call? > > No. > Currently there are multiple constraints and priorities here and adding another > one (a single OpenUrl()) seems like not worth it. First, I am not sure how to do > it without changing the behavior. Second, this function is already quite > complex, I would rather vote for not combining cases in future when we add more > categories (e.g. switch per windowOpenDisposition and inside handle each > category explicitly). Well, what are those constraints? Right now (in the latest patch set) we have first a check on the disposition, then a check on the type of the article, then disposition again, and then type of article again, so if there are any dependencies here that mean those checks have to be in that exact order, that is not explained anywhere. I get that the function is already complex, but I'm trying to make it less complex and less prone to errors. I'm not sure that the current behavior is actually correct -- for example, do we not want to set the referrer when triggering a download? And even if that is a deliberate product decision, how do I know from reading the code that that is the case and not just a product of the seemingly random order in which the checks here are made?
Hi Bernhard, I replied to your comment, please have a look. https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:363: DownloadUtils.openFile( > Well, what are those constraints? E.g. consider downloading before opening an offline page, opening a recent tab before opening an offline page etc. Would you like me to have explicit |if| for each disposition and there handle each category independently (module same code)? BTW, we do not set a referrer when triggering a download (it is handled in a first |if|, which has |return|), but this just illustrates complexity of this function.
https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:363: DownloadUtils.openFile( On 2016/11/17 14:18:47, vitaliii wrote: > > Well, what are those constraints? > E.g. consider downloading before opening an offline page, opening a recent tab > before opening an offline page etc. Actually, both of these are _not_ real constraints -- we can't download an offline page, and recent tabs are a different category from offline pages, so for those specific examples, the order does not matter. > Would you like me to have explicit |if| for each disposition and there handle > each category independently (module same code)? I think what I would like to do is handle everything that does not start a navigation first (opening recent tabs or downloads) and then handle the navigation, by adjusting the LoadUrlParams as desired, and running any additional code (like monitoring the visit). That is why I think this should all be possible with a single LoadUrl() call. > BTW, we do not set a referrer when triggering a download (it is handled in a > first |if|, which has |return|), but this just illustrates complexity of this > function. I'm aware of that. What is not clear though (in particular from the current implementation) is whether that is desired behavior or a bug.
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Hi Bernhard, please have a look. One openUrl now, woo! https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:363: DownloadUtils.openFile( On 2016/11/17 14:50:52, Bernhard Bauer wrote: > On 2016/11/17 14:18:47, vitaliii wrote: > > > Well, what are those constraints? > > E.g. consider downloading before opening an offline page, opening a recent tab > > before opening an offline page etc. > > Actually, both of these are _not_ real constraints -- we can't download an > offline page, and recent tabs are a different category from offline pages, so > for those specific examples, the order does not matter. > > > Would you like me to have explicit |if| for each disposition and there handle > > each category independently (module same code)? > > I think what I would like to do is handle everything that does not start a > navigation first (opening recent tabs or downloads) and then handle the > navigation, by adjusting the LoadUrlParams as desired, and running any > additional code (like monitoring the visit). That is why I think this should all > be possible with a single LoadUrl() call. > > > BTW, we do not set a referrer when triggering a download (it is handled in a > > first |if|, which has |return|), but this just illustrates complexity of this > > function. > > I'm aware of that. What is not clear though (in particular from the current > implementation) is whether that is desired behavior or a bug. Done. https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:398: CHROME_CONTENT_SUGGESTIONS_REFERRER, Referrer.REFERRER_POLICY_ALWAYS)); So do we want a referrer when downloading?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! That does look nicer, I think. https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:358: && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK Do we even offer these dispositions for downloaded assets? https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:383: && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK Same here -- can this actually happen? https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:398: CHROME_CONTENT_SUGGESTIONS_REFERRER, Referrer.REFERRER_POLICY_ALWAYS)); On 2016/11/17 15:47:41, vitaliii wrote: > So do we want a referrer when downloading? I would be in favor, just for the sake of consistency -- if we are going to send a request, we should set the same referrer.
PTAL. https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:358: && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK On 2016/11/17 16:09:59, Bernhard Bauer wrote: > Do we even offer these dispositions for downloaded assets? No. https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:383: && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK On 2016/11/17 16:09:59, Bernhard Bauer wrote: > Same here -- can this actually happen? Yes (e.g. offlined article, Patrick told to open normal incognito when requested incognito tab) https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:398: CHROME_CONTENT_SUGGESTIONS_REFERRER, Referrer.REFERRER_POLICY_ALWAYS)); On 2016/11/17 16:09:59, Bernhard Bauer wrote: > On 2016/11/17 15:47:41, vitaliii wrote: > > So do we want a referrer when downloading? > > I would be in favor, just for the sake of consistency -- if we are going to send > a request, we should set the same referrer. I am not familiar with this topic, so I am fine with your opinion. Currently it is set. Would you like a comment that this is intended? BTW does the referrer change something in case of incognito? Don't we leak anything?
https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:358: && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK On 2016/11/17 16:13:38, vitaliii wrote: > On 2016/11/17 16:09:59, Bernhard Bauer wrote: > > Do we even offer these dispositions for downloaded assets? > > No. OK, so given that we have an assert for the allowed dispositions below, just remove the check then? https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:383: && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK On 2016/11/17 16:13:38, vitaliii wrote: > On 2016/11/17 16:09:59, Bernhard Bauer wrote: > > Same here -- can this actually happen? > > Yes (e.g. offlined article, Patrick told to open normal incognito when requested > incognito tab) OK, thanks. That I think would warrant a comment. https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:398: CHROME_CONTENT_SUGGESTIONS_REFERRER, Referrer.REFERRER_POLICY_ALWAYS)); On 2016/11/17 16:13:38, vitaliii wrote: > On 2016/11/17 16:09:59, Bernhard Bauer wrote: > > On 2016/11/17 15:47:41, vitaliii wrote: > > > So do we want a referrer when downloading? > > > > I would be in favor, just for the sake of consistency -- if we are going to > send > > a request, we should set the same referrer. > > I am not familiar with this topic, so I am fine with your opinion. > Currently it is set. Would you like a comment that this is intended? I think if we *don't* have any special code here, it's fine without a comment. > BTW does the referrer change something in case of incognito? Don't we leak > anything? Incognito mode means not storing any information about the browsing session *locally* after it's closed. It specifically does not turn off sending referrers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:358: && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK On 2016/11/17 16:19:44, Bernhard Bauer wrote: > On 2016/11/17 16:13:38, vitaliii wrote: > > On 2016/11/17 16:09:59, Bernhard Bauer wrote: > > > Do we even offer these dispositions for downloaded assets? > > > > No. > > OK, so given that we have an assert for the allowed dispositions below, just > remove the check then? Done. https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:383: && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK On 2016/11/17 16:19:44, Bernhard Bauer wrote: > On 2016/11/17 16:13:38, vitaliii wrote: > > On 2016/11/17 16:09:59, Bernhard Bauer wrote: > > > Same here -- can this actually happen? > > > > Yes (e.g. offlined article, Patrick told to open normal incognito when > requested > > incognito tab) > > OK, thanks. That I think would warrant a comment. Done. https://codereview.chromium.org/2507023003/diff/70002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:398: CHROME_CONTENT_SUGGESTIONS_REFERRER, Referrer.REFERRER_POLICY_ALWAYS)); On 2016/11/17 16:19:44, Bernhard Bauer wrote: > On 2016/11/17 16:13:38, vitaliii wrote: > > On 2016/11/17 16:09:59, Bernhard Bauer wrote: > > > On 2016/11/17 15:47:41, vitaliii wrote: > > > > So do we want a referrer when downloading? > > > > > > I would be in favor, just for the sake of consistency -- if we are going to > > send > > > a request, we should set the same referrer. > > > > I am not familiar with this topic, so I am fine with your opinion. > > Currently it is set. Would you like a comment that this is intended? > > I think if we *don't* have any special code here, it's fine without a comment. > > > BTW does the referrer change something in case of incognito? Don't we leak > > anything? > > Incognito mode means not storing any information about the browsing session > *locally* after it's closed. It specifically does not turn off sending > referrers. Acknowledged.
LGTM, thanks!
The CQ bit was unchecked by vitaliii@chromium.org
The CQ bit was checked by vitaliii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== [NTP] Implement reactions to snippet context menu actions. Previously a lot of context menu options were not implemented. This CL implements them. BUG=664147,663378,664152 ========== to ========== [NTP] Implement reactions to snippet context menu actions. Previously a lot of context menu options were not implemented. This CL implements them. BUG=664147,663378,664152 Committed: https://crrev.com/6699cc3c380b285d52ed7ce08b63738795876d92 Cr-Commit-Position: refs/heads/master@{#432902} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6699cc3c380b285d52ed7ce08b63738795876d92 Cr-Commit-Position: refs/heads/master@{#432902} |
