Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
248963004:
"Go back" on the block interstitial closes the tab if there is no previous URL. (Closed)
|
Created:
6 years, 8 months ago by Marc Treib Modified:
6 years, 7 months ago CC:
chromium-reviews, jam, pam+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPressing the "Go back" button on the managed user block interstitial page closes the tab if there is no previous URL to go back to.
BUG=307215
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266982
Patch Set 1 #Patch Set 2 : Improved test. #
Total comments: 12
Patch Set 3 : Bernhard's comments. #Patch Set 4 : Typo fix. (Also rebase) #
Messages
Total messages: 33 (0 generated)
Try failures unrelated. phajdan.jr: Please review changes in test_navigation_observer.cc/h. bauerb: Please review changes in managed_mode_interstitial.cc and managed_mode_browsertest.cc. https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... File chrome/browser/managed_mode/managed_mode_interstitial.cc (right): https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... chrome/browser/managed_mode/managed_mode_interstitial.cc:49: delete this; Unrelated question: AFAIK, deleting an object in the constructor, i.e. before it is fully constructed, gives undefined behavior (provided there is a non-trivial destructor, which is the case here since our dtor is virtual). Am I missing something?! https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... File content/public/test/test_navigation_observer.cc (right): https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... content/public/test/test_navigation_observer.cc:129: navigation_started_ = true; The situation where this is required is as follows: We open a link in a new tab, here via ui_test_utils::NavigateToURLWithDisposition(..., NEW_FOREGROUND_TAB, ...). We miss the DidStartLoading, since there's no way to attach the observer before that happens. If the navigation is intercepted by an interstitial, we also don't get a NavigationEntryCommitted, so navigation_started_ is never set to true.
https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... File chrome/browser/managed_mode/managed_mode_browsertest.cc (right): https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... chrome/browser/managed_mode/managed_mode_browsertest.cc:208: tab_strip->AddObserver(&observer); You could move the AddObserver() and RemoveObserver() calls into the constructor and destructor of MockTabStripModelObserver, to make sure that they will always get called. https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... File chrome/browser/managed_mode/managed_mode_interstitial.cc (right): https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... chrome/browser/managed_mode/managed_mode_interstitial.cc:49: delete this; On 2014/04/25 08:12:38, treib wrote: > Unrelated question: AFAIK, deleting an object in the constructor, i.e. before it > is fully constructed, gives undefined behavior (provided there is a non-trivial > destructor, which is the case here since our dtor is virtual). Am I missing > something?! Hm, I think in this case the object is fully constructed (all the fields are initialized), which would explain why it doesn't crash. You're absolutely right though that we shouldn't do this. I would probably move the body of this constructor into an Init() method. We could also make the constructor private and instead expose a static method ManagedModeInterstitial::Show() that creates a new instance, calls Init() on it and returns void. https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... chrome/browser/managed_mode/managed_mode_interstitial.cc:213: if (!continue_request && nav_entry_count == 0) { Nit: braces are optional for one-line bodies in general, and in this file they are left out. https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... File content/public/test/test_navigation_observer.cc (right): https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... content/public/test/test_navigation_observer.cc:127: // Going to an interstitial page does not trigger NavigationEntryCommited, but "NavigationEntryCommitted"
LGTM though. The constructor cleanup is unrelated (feel free to do it in a different CL, or in this one).
Thanks for the comments! https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... File chrome/browser/managed_mode/managed_mode_browsertest.cc (right): https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... chrome/browser/managed_mode/managed_mode_browsertest.cc:208: tab_strip->AddObserver(&observer); On 2014/04/25 08:30:13, Bernhard Bauer wrote: > You could move the AddObserver() and RemoveObserver() calls into the constructor > and destructor of MockTabStripModelObserver, to make sure that they will always > get called. Done. https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... File chrome/browser/managed_mode/managed_mode_interstitial.cc (right): https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... chrome/browser/managed_mode/managed_mode_interstitial.cc:49: delete this; On 2014/04/25 08:30:13, Bernhard Bauer wrote: > On 2014/04/25 08:12:38, treib wrote: > > Unrelated question: AFAIK, deleting an object in the constructor, i.e. before > it > > is fully constructed, gives undefined behavior (provided there is a > non-trivial > > destructor, which is the case here since our dtor is virtual). Am I missing > > something?! > > Hm, I think in this case the object is fully constructed (all the fields are > initialized), which would explain why it doesn't crash. You're absolutely right > though that we shouldn't do this. I would probably move the body of this > constructor into an Init() method. We could also make the constructor private > and instead expose a static method ManagedModeInterstitial::Show() that creates > a new instance, calls Init() on it and returns void. Officially, the object is fully constructed only when we leave the constructor (afaik). I'm not worried about a crash in this case, but rather the compiler deciding "hey, this case can never happen" and removing the whole if. There was a similar problem a while back in the Linux kernel. I can't find anything directly on that particular problem right now, but compare http://blog.llvm.org/2011/05/what-every-c-programmer-should-know_14.html (first example). Anyway, I'll clean this up in a follow-up CL. https://codereview.chromium.org/248963004/diff/20001/chrome/browser/managed_m... chrome/browser/managed_mode/managed_mode_interstitial.cc:213: if (!continue_request && nav_entry_count == 0) { On 2014/04/25 08:30:13, Bernhard Bauer wrote: > Nit: braces are optional for one-line bodies in general, and in this file they > are left out. Done. https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... File content/public/test/test_navigation_observer.cc (right): https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... content/public/test/test_navigation_observer.cc:127: // Going to an interstitial page does not trigger NavigationEntryCommited, but On 2014/04/25 08:30:13, Bernhard Bauer wrote: > "NavigationEntryCommitted" Done.
LGTM
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/248963004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/248963004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/248963004/40001
https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... File content/public/test/test_navigation_observer.cc (right): https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... content/public/test/test_navigation_observer.cc:127: // Going to an interstitial page does not trigger NavigationEntryCommited, but On 2014/04/25 08:50:34, treib wrote: > On 2014/04/25 08:30:13, Bernhard Bauer wrote: > > "NavigationEntryCommitted" > > Done. No, I meant that "committed" is spelled with double-t :) The quote were not meant for the code.
The CQ bit was unchecked by treib@chromium.org
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/248963004/60001
https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... File content/public/test/test_navigation_observer.cc (right): https://codereview.chromium.org/248963004/diff/20001/content/public/test/test... content/public/test/test_navigation_observer.cc:127: // Going to an interstitial page does not trigger NavigationEntryCommited, but On 2014/04/29 07:57:37, Bernhard Bauer wrote: > On 2014/04/25 08:50:34, treib wrote: > > On 2014/04/25 08:30:13, Bernhard Bauer wrote: > > > "NavigationEntryCommitted" > > > > Done. > > No, I meant that "committed" is spelled with double-t :) The quote were not > meant for the code. Might have made that more obvious :P Anyway, juust in time!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/248963004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/248963004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/248963004/60001
Message was sent while issue was closed.
Change committed as 266982 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
