Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4647007:
Fix navigation bugs, patch the right file, nuke an old file. (Closed)
|
Created:
10 years, 1 month ago by sadrul Modified:
9 years, 6 months ago CC:
chromium-reviews, ben+cc_chromium.org, Aaron Boodman, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix some navigation bugs.
Set ADD_SELECTED to foreground/singleton tabs, and set show_window when a new window is created during Navigate().
BUG=62022, 62137, 62545, 62923, 63019
TEST=see bugs
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66457
Patch Set 1 #
Total comments: 5
Patch Set 2 : Update header files, add comments, nuke old header. #Patch Set 3 : Include a fix for 62137 #Patch Set 4 : Add fix for 62545 #
Total comments: 5
Patch Set 5 : Set show_window more conservatively. #Patch Set 6 : merge with trunk #
Total comments: 4
Patch Set 7 : Updated comments as per Ben's comments. #Messages
Total messages: 25 (0 generated)
LGTM but I assume you'll trybot it and wait for Ben's ok too since I'm not that clued up about the code in question. Thanks for sorting this out! Nit: the description might want to mention 'open link in incognito' rather than 'a bug'. ( I saw in the other review btw. that you were waiting for the trybots to get to a later revision - you can force them to try against a specific revision with -r revno IIRC )
On 2010/11/09 20:25:14, Craig wrote: > LGTM but I assume you'll trybot it and wait for Ben's ok too since I'm not that > clued up about the code in question. Thanks for sorting this out! Nit: the > description might want to mention 'open link in incognito' rather than 'a bug'. Ah wait, you did mention incognito. Ignore the nit.
http://codereview.chromium.org/4647007/diff/1/3 File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/1/3#newcode189 chrome/browser/ui/browser_navigator.cc:189: else if (params->disposition == NEW_FOREGROUND_TAB) maybe NEW_FOREGROUND_TAB can be added to the dispositions below on L194. Also, can you update the comment in the header about this?
I have added comments in the header file. I removed the old header file and updated the #include's accordingly. http://codereview.chromium.org/4647007/diff/1/3 File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/1/3#newcode189 chrome/browser/ui/browser_navigator.cc:189: else if (params->disposition == NEW_FOREGROUND_TAB) On 2010/11/09 20:29:23, Evan Stade wrote: > maybe NEW_FOREGROUND_TAB can be added to the dispositions below on L194. Also, > can you update the comment in the header about this? I thought about doing a fallthrough switch-case on this: switch (params->disposition) { case NEW_BACKGROUND_TAB: params->tabstrip_add_types &= ~TabStripModel::ADD_SELECTED; break; case NEW_WINDOW: case NEW_POPUP: params->show_window = true; // Fall-through case NEW_FOREGROUND_TAB: params->tabstrip_add_types |= TabStripModel::ADD_SELECTED; break; default: break; } How does that sound?
[snip] > ( I saw in the other review btw. that you were waiting for the trybots to get to > a later revision - you can force them to try against a specific revision with -r > revno IIRC ) I got the trybots using a newer revision for the previous CL. I forgot to do that for the tryjob with patchset-1 here. I have submitted patchset-2 to the trybots with a newer revision, though.
http://codereview.chromium.org/4647007/diff/1/3 File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/1/3#newcode189 chrome/browser/ui/browser_navigator.cc:189: else if (params->disposition == NEW_FOREGROUND_TAB) On 2010/11/09 20:50:54, sadrul wrote: > On 2010/11/09 20:29:23, Evan Stade wrote: > > maybe NEW_FOREGROUND_TAB can be added to the dispositions below on L194. Also, > > can you update the comment in the header about this? > > I thought about doing a fallthrough switch-case on this: > > switch (params->disposition) { > case NEW_BACKGROUND_TAB: > params->tabstrip_add_types &= ~TabStripModel::ADD_SELECTED; > break; > case NEW_WINDOW: > case NEW_POPUP: > params->show_window = true; > // Fall-through > case NEW_FOREGROUND_TAB: > params->tabstrip_add_types |= TabStripModel::ADD_SELECTED; > break; > > default: > break; > } > > How does that sound? what's wrong with showing the window for NEW_FOREGROUND? I think Show is harmless if the window's already showing (actually, it probably Activates the window, which seems desirable). if there were a context menu item like "open in incognito" but instead it were called "open in normal", and you didn't have any normal windows when you selected it in an OTR window, you'd want to show the new window you had to create.
http://codereview.chromium.org/4647007/diff/1/3 File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/1/3#newcode189 chrome/browser/ui/browser_navigator.cc:189: else if (params->disposition == NEW_FOREGROUND_TAB) [snip] > what's wrong with showing the window for NEW_FOREGROUND? I think Show is > harmless if the window's already showing (actually, it probably Activates the > window, which seems desirable). > > if there were a context menu item like "open in incognito" but instead it were > called "open in normal", and you didn't have any normal windows when you > selected it in an OTR window, you'd want to show the new window you had to > create. That makes sense to me (and |show_window| does activate the window if already exists). However, I do not know what 'activate the window' may mean for different window-managers, and I am not familiar with this code enough to know if this can cause problems for some. But if it sounds like an OK change to you/Ben, I do not personally have a problem with it.
This new CL includes a fix for 62137 http://codereview.chromium.org/4647007/diff/1/chrome/browser/ui/browser_navig... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/1/chrome/browser/ui/browser_navig... chrome/browser/ui/browser_navigator.cc:189: else if (params->disposition == NEW_FOREGROUND_TAB) On 2010/11/09 21:06:39, sadrul wrote: > [snip] > > what's wrong with showing the window for NEW_FOREGROUND? I think Show is > > harmless if the window's already showing (actually, it probably Activates the > > window, which seems desirable). > > > > if there were a context menu item like "open in incognito" but instead it were > > called "open in normal", and you didn't have any normal windows when you > > selected it in an OTR window, you'd want to show the new window you had to > > create. > > That makes sense to me (and |show_window| does activate the window if already > exists). However, I do not know what 'activate the window' may mean for > different window-managers, and I am not familiar with this code enough to know > if this can cause problems for some. But if it sounds like an OK change to > you/Ben, I do not personally have a problem with it. I have gone ahead and made this change. This essentially makes a single change fix both 62022 and 62137!
Setting ADD_SELECTED for SINGLETON_TABs fixes crbug 62545. So I have included this fix in this CL too. I believe this takes care of the regression bugs related to navigation: http://code.google.com/p/chromium/issues/list?can=1&q=64745 (61735 and 62342 were fixed by http://codereview.chromium.org/4534002/) PTAL!
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.cc:193: case NEW_FOREGROUND_TAB: What are the other implications of this? There are several cases on startup where we create a window, add a foreground tab to it, do some other work and then show the window. This makes it automatic and thus changes the show order.
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.cc:193: case NEW_FOREGROUND_TAB: On 2010/11/10 18:43:38, Ben Goodger wrote: > What are the other implications of this? There are several cases on startup > where we create a window, add a foreground tab to it, do some other work and > then show the window. This makes it automatic and thus changes the show order. Are there tests/scenarios I could try to reproduce those cases? Perhaps |show_window| could be an enum (UNDECIDED, SHOW, DONOT_SHOW), and in these cases, it will be set to DONOT_SHOW, in which case it will not be set to SHOW in NormalizeDisposition.
I would say audit all the places that pass NEW_FOREGROUND_TAB as the disposition. On Wed, Nov 10, 2010 at 10:50 AM, <sadrul@chromium.org> wrote: > > > http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... > File chrome/browser/ui/browser_navigator.cc (right): > > > http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... > chrome/browser/ui/browser_navigator.cc:193: case NEW_FOREGROUND_TAB: > On 2010/11/10 18:43:38, Ben Goodger wrote: > >> What are the other implications of this? There are several cases on >> > startup > >> where we create a window, add a foreground tab to it, do some other >> > work and > >> then show the window. This makes it automatic and thus changes the >> > show order. > > Are there tests/scenarios I could try to reproduce those cases? Perhaps > |show_window| could be an enum (UNDECIDED, SHOW, DONOT_SHOW), and in > these cases, it will be set to DONOT_SHOW, in which case it will not be > set to SHOW in NormalizeDisposition. > > > http://codereview.chromium.org/4647007/ >
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.cc:201: default: Can you deal with OFF_THE_RECORD mode too? I was going to fix it, but it looks like you're moving everything so I won't try and complicate your life more. The specific use case to test for is right click on a link and chose open in incognito window. Currently we drop it on the floor (or rather create a browser but never show it).
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.cc:201: default: On 2010/11/10 19:18:47, sky wrote: > Can you deal with OFF_THE_RECORD mode too? I was going to fix it, but it looks > like you're moving everything so I won't try and complicate your life more. > > The specific use case to test for is right click on a link and chose open in > incognito window. Currently we drop it on the floor (or rather create a browser > but never show it). I think you are referring to bug 62022? In which case, this CL fixes that problem too.
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.cc:201: default: 2010/11/10 19:25:08, sadrul wrote: > On 2010/11/10 19:18:47, sky wrote: > > Can you deal with OFF_THE_RECORD mode too? I was going to fix it, but it looks > > like you're moving everything so I won't try and complicate your life more. > > > > The specific use case to test for is right click on a link and chose open in > > incognito window. Currently we drop it on the floor (or rather create a > browser > > but never show it). > > I think you are referring to bug 62022? In which case, this CL fixes that > problem too. Indeed I am. Glad to hear it!
On 2010/11/10 19:13:49, Ben Goodger wrote: > I would say audit all the places that pass NEW_FOREGROUND_TAB as the > disposition. I have changed the logic a little: * If a new window is created, then set |show_window| to true. * For NEW_WINDOW and NEW_POPUP, set |show_window| to true. So |show_window| is not set for every NEW_FOREGROUND_TAB anymore. This CL still fixes all three bugs. WDYT?
On 2010/11/11 03:43:39, sadrul wrote: > On 2010/11/10 19:13:49, Ben Goodger wrote: > > I would say audit all the places that pass NEW_FOREGROUND_TAB as the > > disposition. > > I have changed the logic a little: > * If a new window is created, then set |show_window| to true. I haven't stared at this too hard but I wonder if that logic will cover the NEW_WINDOW and NEW_POPUP cases too? > * For NEW_WINDOW and NEW_POPUP, set |show_window| to true. i.e. that maybe this is unnecessary with the "if a new window is created" change?? > So |show_window| is not set for every NEW_FOREGROUND_TAB anymore. This CL still > fixes all three bugs. WDYT? It seems reasonable to me but this stuff is subtle so I still think we need someone with experience in this area to weigh in. Moar review comments please!
Looks like http://codereview.chromium.org/4840001 took care of the #include-updates. Updated this CL after merging with trunk.
ping Is someone still going to review this or has it dropped off the radar?
I believe Ben is going to review it. -Scott On Wed, Nov 17, 2010 at 7:48 AM, <craig.schlenter@chromium.org> wrote: > ping > > Is someone still going to review this or has it dropped off the radar? > > http://codereview.chromium.org/4647007/ >
http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.cc:303: params->show_window = true; Did you evaluate all cases relating to startup (in browser.cc, browser_init.cc etc) and verify that no one did any work post-Navigate() but before calling browser->window()->Show()? http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.h (right): http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.h:85: // If disposition is one of NEW_WINDOW, NEW_POPUP, NEW_FOREGROUND_TAB or The list of dispositions here now that coerce show_window no longer match that in the switch statement in the .cc file. Also you should explain next to show_window the conditions that lead to its coercion.
http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.cc:303: params->show_window = true; On 2010/11/17 15:57:50, Ben Goodger wrote: > Did you evaluate all cases relating to startup (in browser.cc, browser_init.cc > etc) and verify that no one did any work post-Navigate() but before calling > browser->window()->Show()? I cannot honestly claim I covered all the places, but I tried my best. It looks like window()->Show() is called (if at all) immediately after Navigate() (or after AddBlankTab() etc. which essentially ends with Navigate()). BrowserInit::LaunchWithProfile::OpenTabsInBrowser does a number of Navigate() calls in a loop, and then calls Show(). But this change shouldn't affect that, and |show_window| shouldn't be set since params->browser wouldn't be changed within GetBrowserForDisposition or AdjustNavigateParamsForURL. http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_n... File chrome/browser/ui/browser_navigator.h (right): http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_n... chrome/browser/ui/browser_navigator.h:85: // If disposition is one of NEW_WINDOW, NEW_POPUP, NEW_FOREGROUND_TAB or On 2010/11/17 15:57:50, Ben Goodger wrote: > The list of dispositions here now that coerce show_window no longer match that > in the switch statement in the .cc file. > > Also you should explain next to show_window the conditions that lead to its > coercion. Done.
LGTM then... let's keep an eye out for bugs.
On 2010/11/17 17:25:23, Ben Goodger wrote: > LGTM then... let's keep an eye out for bugs. Will do. Thanks! :) |
