Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2256383002:
Add PreviewsInfoBarTabHelper (Closed)
DescriptionAdd PreviewsInfoBarTabHelper
PreviewsInfoBarTabHelper tracks whether a previews infobar
has been shown for a tab. If a previews infobar has already
been shown for a tab, it will not be shown again.
BUG=615566
Committed: https://crrev.com/1d778dbcf8cac9d2ab2ef10959f315ad394aa1ab
Cr-Commit-Position: refs/heads/master@{#418588}
Patch Set 1 #
Total comments: 2
Patch Set 2 : avi comment #
Total comments: 16
Patch Set 3 : fix gn #Patch Set 4 : bengr comments #
Total comments: 2
Patch Set 5 : fix comment #Patch Set 6 : rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 44 (29 generated)
Description was changed from ========== Add PreviewsInfoBarTabHelper Add PreviewsInfoBarTabHelper to track whether a previews infobar has been shown for a tab. BUG=615566 ========== to ========== Add PreviewsInfoBarTabHelper PreviewsInfoBarTabHelper tracks whether a previews infobar has been shown for a tab. If a infobar has already been shown for a tab, it will not be shown again. BUG=615566 ==========
Description was changed from ========== Add PreviewsInfoBarTabHelper PreviewsInfoBarTabHelper tracks whether a previews infobar has been shown for a tab. If a infobar has already been shown for a tab, it will not be shown again. BUG=615566 ========== to ========== Add PreviewsInfoBarTabHelper PreviewsInfoBarTabHelper tracks whether a previews infobar has been shown for a tab. If a previews infobar has already been shown for a tab, it will not be shown again. BUG=615566 ==========
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
megjablon@chromium.org changed reviewers: + avi@chromium.org, bengr@chromium.org
avi: tab_helpers.cc bengr: previews
https://chromiumcodereview.appspot.com/2256383002/diff/60001/chrome/browser/u... File chrome/browser/ui/tab_helpers.cc (right): https://chromiumcodereview.appspot.com/2256383002/diff/60001/chrome/browser/u... chrome/browser/ui/tab_helpers.cc:189: PreviewsInfoBarTabHelper::CreateForWebContents(web_contents); alphabetical, plz
https://chromiumcodereview.appspot.com/2256383002/diff/60001/chrome/browser/u... File chrome/browser/ui/tab_helpers.cc (right): https://chromiumcodereview.appspot.com/2256383002/diff/60001/chrome/browser/u... chrome/browser/ui/tab_helpers.cc:189: PreviewsInfoBarTabHelper::CreateForWebContents(web_contents); On 2016/08/30 18:38:16, Avi wrote: > alphabetical, plz Done.
tab helpers lgtm
https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:25: if (infobar_tab_helper->DisplayedPreviewInfoBar()) Can infobar_tab_helper be null? https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_tab_helper.cc (right): https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.cc:40: if (!render_frame_host->GetParent()) { Remove curly braces. https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.cc:51: if (headers && headers->HasHeaderValue( Indentation is off. https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_tab_helper.h (right): https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.h:19: // infobar for LitePages when the main frame response came back with a This seems like an implementation detail. How about: Handles showing the infobar when the main frame response indicates a Lite Page. https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.h:21: class PreviewsInfoBarTabHelper This class should have tests. https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.h:27: // Indicates whether the InfoBar for Data Reduction Proxy preview has been for -> for the Also, what is a DRP preview and how is it different from a preview shown for a Lite Page? https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.h:29: bool DisplayedPreviewInfoBar() const; These are just simple getters and setters so they can be inlined, use underscore naming, and not be commented: bool displayed_preview_infobar() const { return displayed_preview_infobar_; } void set_displayed_preview_infobar(bool displayed) { displayed_preview_infobar_ = displayed; } https://codereview.chromium.org/2256383002/diff/80001/chrome/browser/previews... chrome/browser/previews/previews_infobar_tab_helper.h:46: // True if the InfoBar for Data Reduction Proxy preview has been shown for for -> for the
Patchset #3 (id:100001) has been deleted
https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... chrome/browser/previews/previews_infobar_delegate.cc:25: if (infobar_tab_helper->DisplayedPreviewInfoBar()) On 2016/08/31 23:27:04, bengr wrote: > Can infobar_tab_helper be null? The PreviewsInfoBarTabHelper is created in AttachTabHelpers(). AttachTabHelpers() is called whenever a WebContents is created for use as a browser tab, so we should always have the helper. https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... File chrome/browser/previews/previews_infobar_tab_helper.cc (right): https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... chrome/browser/previews/previews_infobar_tab_helper.cc:40: if (!render_frame_host->GetParent()) { On 2016/08/31 23:27:04, bengr wrote: > Remove curly braces. Done. https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... chrome/browser/previews/previews_infobar_tab_helper.cc:51: if (headers && headers->HasHeaderValue( On 2016/08/31 23:27:04, bengr wrote: > Indentation is off. Done. https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... File chrome/browser/previews/previews_infobar_tab_helper.h (right): https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... chrome/browser/previews/previews_infobar_tab_helper.h:19: // infobar for LitePages when the main frame response came back with a On 2016/08/31 23:27:04, bengr wrote: > This seems like an implementation detail. How about: > > Handles showing the infobar when the main frame response indicates a Lite Page. Done. https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... chrome/browser/previews/previews_infobar_tab_helper.h:21: class PreviewsInfoBarTabHelper On 2016/08/31 23:27:04, bengr wrote: > This class should have tests. Tests are added in https://chromiumcodereview.appspot.com/2266653002/ Haven't sent this cl out for review because I didn't want to spam a ton of cls at once. I wanted to get the most important cls that would affect how the others are implemented reviewed first. https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... chrome/browser/previews/previews_infobar_tab_helper.h:27: // Indicates whether the InfoBar for Data Reduction Proxy preview has been On 2016/08/31 23:27:04, bengr wrote: > for -> for the > > Also, what is a DRP preview and how is it different from a preview shown for a > Lite Page? Removed. This should just be for all previews, not only drp dependent ones. https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... chrome/browser/previews/previews_infobar_tab_helper.h:29: bool DisplayedPreviewInfoBar() const; On 2016/08/31 23:27:04, bengr wrote: > These are just simple getters and setters so they can be inlined, use underscore > naming, and not be commented: > > bool displayed_preview_infobar() const { > return displayed_preview_infobar_; > } > > void set_displayed_preview_infobar(bool displayed) { > displayed_preview_infobar_ = displayed; > } Done. https://chromiumcodereview.appspot.com/2256383002/diff/80001/chrome/browser/p... chrome/browser/previews/previews_infobar_tab_helper.h:46: // True if the InfoBar for Data Reduction Proxy preview has been shown for On 2016/08/31 23:27:04, bengr wrote: > for -> for the Done.
lgtm https://codereview.chromium.org/2256383002/diff/140001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_tab_helper.h (right): https://codereview.chromium.org/2256383002/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_tab_helper.h:18: // Tracks whether a preview infobar has been shown for a page. Handles showing preview -> previews?
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
https://codereview.chromium.org/2256383002/diff/140001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_tab_helper.h (right): https://codereview.chromium.org/2256383002/diff/140001/chrome/browser/preview... chrome/browser/previews/previews_infobar_tab_helper.h:18: // Tracks whether a preview infobar has been shown for a page. Handles showing On 2016/09/08 00:41:36, bengr wrote: > preview -> previews? Done.
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.
The CQ bit was checked by megjablon@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...
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by megjablon@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.
The CQ bit was checked by megjablon@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 megjablon@chromium.org
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, bengr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2256383002/#ps220001 (title: "rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
megjablon@chromium.org changed reviewers: + jochen@chromium.org
+jochen: BUILD.gn
lgtm
The CQ bit was checked by megjablon@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.
Description was changed from ========== Add PreviewsInfoBarTabHelper PreviewsInfoBarTabHelper tracks whether a previews infobar has been shown for a tab. If a previews infobar has already been shown for a tab, it will not be shown again. BUG=615566 ========== to ========== Add PreviewsInfoBarTabHelper PreviewsInfoBarTabHelper tracks whether a previews infobar has been shown for a tab. If a previews infobar has already been shown for a tab, it will not be shown again. BUG=615566 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add PreviewsInfoBarTabHelper PreviewsInfoBarTabHelper tracks whether a previews infobar has been shown for a tab. If a previews infobar has already been shown for a tab, it will not be shown again. BUG=615566 ========== to ========== Add PreviewsInfoBarTabHelper PreviewsInfoBarTabHelper tracks whether a previews infobar has been shown for a tab. If a previews infobar has already been shown for a tab, it will not be shown again. BUG=615566 Committed: https://crrev.com/1d778dbcf8cac9d2ab2ef10959f315ad394aa1ab Cr-Commit-Position: refs/heads/master@{#418588} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1d778dbcf8cac9d2ab2ef10959f315ad394aa1ab Cr-Commit-Position: refs/heads/master@{#418588} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
