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
Reviewers:
Craig, sky, Ben Goodger (Google), Evan Stade
CC:
chromium-reviews, ben+cc_chromium.org, Aaron Boodman, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

Fix 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. #

Unified diffs Side-by-side diffs Stats (+33 lines, -13 lines)
M chrome/browser/ui/browser_navigator.h View 2 chunks +5 lines, -2 lines 0 comments
M chrome/browser/ui/browser_navigator.cc View 3 chunks +28 lines, -11 lines 0 comments

Messages

Total messages: 25 (0 generated)
sadrul
10 years, 1 month ago (2010-11-09 20:03:06 UTC) #1
Craig
LGTM but I assume you'll trybot it and wait for Ben's ok too since I'm ...
10 years, 1 month ago (2010-11-09 20:25:14 UTC) #2
Craig
On 2010/11/09 20:25:14, Craig wrote: > LGTM but I assume you'll trybot it and wait ...
10 years, 1 month ago (2010-11-09 20:26:31 UTC) #3
Evan Stade
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 ...
10 years, 1 month ago (2010-11-09 20:29:23 UTC) #4
sadrul
I have added comments in the header file. I removed the old header file and ...
10 years, 1 month ago (2010-11-09 20:50:53 UTC) #5
sadrul
[snip] > ( I saw in the other review btw. that you were waiting for ...
10 years, 1 month ago (2010-11-09 20:53:31 UTC) #6
Evan Stade
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 ...
10 years, 1 month ago (2010-11-09 20:56:17 UTC) #7
sadrul
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 ...
10 years, 1 month ago (2010-11-09 21:06:39 UTC) #8
sadrul
This new CL includes a fix for 62137 http://codereview.chromium.org/4647007/diff/1/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/1/chrome/browser/ui/browser_navigator.cc#newcode189 chrome/browser/ui/browser_navigator.cc:189: else ...
10 years, 1 month ago (2010-11-10 05:07:57 UTC) #9
sadrul
Setting ADD_SELECTED for SINGLETON_TABs fixes crbug 62545. So I have included this fix in this ...
10 years, 1 month ago (2010-11-10 16:11:00 UTC) #10
Ben Goodger (Google)
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc#newcode193 chrome/browser/ui/browser_navigator.cc:193: case NEW_FOREGROUND_TAB: What are the other implications of this? ...
10 years, 1 month ago (2010-11-10 18:43:38 UTC) #11
sadrul
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc#newcode193 chrome/browser/ui/browser_navigator.cc:193: case NEW_FOREGROUND_TAB: On 2010/11/10 18:43:38, Ben Goodger wrote: > ...
10 years, 1 month ago (2010-11-10 18:50:30 UTC) #12
Ben Goodger (Google)
I would say audit all the places that pass NEW_FOREGROUND_TAB as the disposition. On Wed, ...
10 years, 1 month ago (2010-11-10 19:13:49 UTC) #13
sky
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc#newcode201 chrome/browser/ui/browser_navigator.cc:201: default: Can you deal with OFF_THE_RECORD mode too? I ...
10 years, 1 month ago (2010-11-10 19:18:47 UTC) #14
sadrul
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc#newcode201 chrome/browser/ui/browser_navigator.cc:201: default: On 2010/11/10 19:18:47, sky wrote: > Can you ...
10 years, 1 month ago (2010-11-10 19:25:08 UTC) #15
sky
http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/18001/chrome/browser/ui/browser_navigator.cc#newcode201 chrome/browser/ui/browser_navigator.cc:201: default: 2010/11/10 19:25:08, sadrul wrote: > On 2010/11/10 19:18:47, ...
10 years, 1 month ago (2010-11-10 19:30:12 UTC) #16
sadrul
On 2010/11/10 19:13:49, Ben Goodger wrote: > I would say audit all the places that ...
10 years, 1 month ago (2010-11-11 03:43:39 UTC) #17
Craig
On 2010/11/11 03:43:39, sadrul wrote: > On 2010/11/10 19:13:49, Ben Goodger wrote: > > I ...
10 years, 1 month ago (2010-11-12 05:16:55 UTC) #18
sadrul
Looks like http://codereview.chromium.org/4840001 took care of the #include-updates. Updated this CL after merging with trunk.
10 years, 1 month ago (2010-11-13 05:56:40 UTC) #19
Craig
ping Is someone still going to review this or has it dropped off the radar?
10 years, 1 month ago (2010-11-17 15:48:43 UTC) #20
sky
I believe Ben is going to review it. -Scott On Wed, Nov 17, 2010 at ...
10 years, 1 month ago (2010-11-17 15:54:23 UTC) #21
Ben Goodger (Google)
http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_navigator.cc#newcode303 chrome/browser/ui/browser_navigator.cc:303: params->show_window = true; Did you evaluate all cases relating ...
10 years, 1 month ago (2010-11-17 15:57:50 UTC) #22
sadrul
http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/4647007/diff/32001/chrome/browser/ui/browser_navigator.cc#newcode303 chrome/browser/ui/browser_navigator.cc:303: params->show_window = true; On 2010/11/17 15:57:50, Ben Goodger wrote: ...
10 years, 1 month ago (2010-11-17 17:02:21 UTC) #23
Ben Goodger (Google)
LGTM then... let's keep an eye out for bugs.
10 years, 1 month ago (2010-11-17 17:25:23 UTC) #24
sadrul
10 years, 1 month ago (2010-11-17 17:42:16 UTC) #25
On 2010/11/17 17:25:23, Ben Goodger wrote:
> LGTM then... let's keep an eye out for bugs.

Will do. Thanks! :)

Powered by Google App Engine