|
|
Created:
8 years, 2 months ago by Dan Beam Modified:
8 years, 2 months ago CC:
chromium-reviews, brettw Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix zoom icon/bubble for virtual URLs (i.e. chrome://settings).
R=khorimoto@chromium.org,sky@chromium.org
BUG=153950
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160228
Patch Set 1 #
Total comments: 5
Patch Set 2 : fix #Patch Set 3 : doc #Messages
Total messages: 14 (0 generated)
+sky for OWNERS
https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... File chrome/browser/ui/zoom/zoom_controller.cc (right): https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... chrome/browser/ui/zoom/zoom_controller.cc:93: } else { When exactly is active_entry NULL? A little confused here.
https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... File chrome/browser/ui/zoom/zoom_controller.cc (right): https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... chrome/browser/ui/zoom/zoom_controller.cc:93: } else { On 2012/10/04 03:00:22, Kyle Horimoto wrote: > When exactly is active_entry NULL? A little confused here. http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/nav...
LGTM https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... File chrome/browser/ui/zoom/zoom_controller.cc (right): https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... chrome/browser/ui/zoom/zoom_controller.cc:90: // Prefer to ask the navigation controller for the non-virtual URL. Document why we need to do this.
https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... File chrome/browser/ui/zoom/zoom_controller.cc (right): https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... chrome/browser/ui/zoom/zoom_controller.cc:90: // Prefer to ask the navigation controller for the non-virtual URL. On 2012/10/04 16:01:12, sky wrote: > Document why we need to do this. Is the explanation a couple lines below sufficient (that virtual URLs break this)?
On 2012/10/04 03:13:04, Dan Beam wrote: > https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... > File chrome/browser/ui/zoom/zoom_controller.cc (right): > > https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... > chrome/browser/ui/zoom/zoom_controller.cc:93: } else { > On 2012/10/04 03:00:22, Kyle Horimoto wrote: > > When exactly is active_entry NULL? A little confused here. > > http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/nav... I saw that code comment before I replied, but I'm still wondering when it can be NULL. For what situations does web_contents()->GetURL() work when web_contents()->GetController().GetActiveEntry()->GetURL() doesn't?
On 2012/10/04 16:49:18, Kyle Horimoto wrote: > On 2012/10/04 03:13:04, Dan Beam wrote: > > > https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... > > File chrome/browser/ui/zoom/zoom_controller.cc (right): > > > > > https://codereview.chromium.org/11040031/diff/1/chrome/browser/ui/zoom/zoom_c... > > chrome/browser/ui/zoom/zoom_controller.cc:93: } else { > > On 2012/10/04 03:00:22, Kyle Horimoto wrote: > > > When exactly is active_entry NULL? A little confused here. > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/nav... > > I saw that code comment before I replied, but I'm still wondering when it can be > NULL. For what situations does web_contents()->GetURL() work when > web_contents()->GetController().GetActiveEntry()->GetURL() doesn't? I don't know, ask brettw@ (I already asked jam@)
LGTM Looks fine to me assuming this behaves how we think it does - I'll ask brettw@.
The active entry can be null in some cases like if a popup is opened with about blank and the original page starts writing HTML to it. As documented in GetActiveEntry, you definitely want the null check, but I'm not sure what the best thing for your code to do in the null case. The GetURL probably returns either empty or "about:blank" (I forget which one) when this happens.
On 2012/10/04 18:18:55, brettw wrote: > The active entry can be null in some cases like if a popup is opened with about > blank and the original page starts writing HTML to it. As documented in > GetActiveEntry, you definitely want the null check, but I'm not sure what the > best thing for your code to do in the null case. The GetURL probably returns > either empty or "about:blank" (I forget which one) when this happens. Cool, thanks for the explanation, Brett!
https://chromiumcodereview.appspot.com/11040031/diff/1/chrome/browser/ui/zoom... File chrome/browser/ui/zoom/zoom_controller.cc (right): https://chromiumcodereview.appspot.com/11040031/diff/1/chrome/browser/ui/zoom... chrome/browser/ui/zoom/zoom_controller.cc:90: // Prefer to ask the navigation controller for the non-virtual URL. On 2012/10/04 16:03:11, Dan Beam wrote: > On 2012/10/04 16:01:12, sky wrote: > > Document why we need to do this. > > Is the explanation a couple lines below sufficient (that virtual URLs break > this)? OK, so I added a comment in the newest version with a link to the bug.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/11040031/3
Change committed as 160228 |