Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
301243018:
Fix issue with Virtual vs. real URLs. (Closed)
|
Created:
6 years, 6 months ago by wjmaclean Modified:
6 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, Fady Samuel Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix issue with Virtual vs. real URLs.
In r273854 a regression was introduced in which
HostZoomMapImpl::SetZoomLevelForWebContents() used a virtual url for
setting page zoom, when the real url is required. This CL fixes that
by calling GetLastCommittedEntry() on the WebContents navigation
controller, instead of calling it directly on the WebContents; the
latter returns a virtual url.
BUG=379622
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275268
Patch Set 1 #
Total comments: 1
Patch Set 2 : Address comments. #
Total comments: 3
Patch Set 3 : Modify entry checking in SetZoomLevel(). #
Total comments: 1
Patch Set 4 : Remove extra spaces. #Messages
Total messages: 16 (0 generated)
Not sure who to ask for a review here, so please feel free to pass this along if I'm wrong.
https://codereview.chromium.org/301243018/diff/1/content/browser/host_zoom_ma... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/301243018/diff/1/content/browser/host_zoom_ma... content/browser/host_zoom_map_impl.cc:245: web_contents_impl.GetController().GetLastCommittedEntry()->GetURL(); (1) Are we guaranteed to have a committed entry by the time we reach here? (2) This should perhaps have a comment about why WebContentsImpl::GetLastCommittedURL() is wrong.
On 2014/06/02 20:38:53, Peter Kasting wrote: > https://codereview.chromium.org/301243018/diff/1/content/browser/host_zoom_ma... > File content/browser/host_zoom_map_impl.cc (right): > > https://codereview.chromium.org/301243018/diff/1/content/browser/host_zoom_ma... > content/browser/host_zoom_map_impl.cc:245: > web_contents_impl.GetController().GetLastCommittedEntry()->GetURL(); > (1) Are we guaranteed to have a committed entry by the time we reach here? Good point, check added. > (2) This should perhaps have a comment about why > WebContentsImpl::GetLastCommittedURL() is wrong. Done. I also rewrote the comment in the Get function to be consistent.
https://codereview.chromium.org/301243018/diff/40001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/301243018/diff/40001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:230: if (entry) So, when I asked if we were guaranteed a committed entry on reaching this point (in Set...(), but whatever), I was legitimately unsure. This code tells the reader it is definitely possible to reach here with no such entry, which doesn't seem completely impossible, but certainly surprises me. We should only have these sorts of conditionals if this is really possible, and if it is, it would be very useful to put comments in saying when this can be (e.g. during tests, on opening a brand new tab, during browser startup, etc.). https://codereview.chromium.org/301243018/diff/40001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:231: url = entry->GetURL(); Nit: Two spaces (2 places) https://codereview.chromium.org/301243018/diff/40001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:252: if (entry) If this is NULL, do we really want to set the zoom level for GURL()?
On 2014/06/02 21:46:55, Peter Kasting wrote: > https://codereview.chromium.org/301243018/diff/40001/content/browser/host_zoo... > File content/browser/host_zoom_map_impl.cc (right): > > https://codereview.chromium.org/301243018/diff/40001/content/browser/host_zoo... > content/browser/host_zoom_map_impl.cc:230: if (entry) > So, when I asked if we were guaranteed a committed entry on reaching this point > (in Set...(), but whatever), I was legitimately unsure. This code tells the > reader it is definitely possible to reach here with no such entry, which doesn't > seem completely impossible, but certainly surprises me. We should only have > these sorts of conditionals if this is really possible, and if it is, it would > be very useful to put comments in saying when this can be (e.g. during tests, on > opening a brand new tab, during browser startup, etc.). OK. I'll see if I can figure out if it is or isn't possible. If I'm still unsre, is a DCHECK ok for now?
On 2014/06/02 22:11:09, wjmaclean wrote: > OK. I'll see if I can figure out if it is or isn't possible. If I'm still unsre, > is a DCHECK ok for now? If you're not sure, and you're hoping to get data to inform things, you might want a CHECK instead of a DCHECK, so that having a NULL entry will cause an instant crash. Then you'd want to change this to a DCHECK if it seemed to work correctly.
On 2014/06/02 22:13:08, Peter Kasting wrote: > On 2014/06/02 22:11:09, wjmaclean wrote: > > OK. I'll see if I can figure out if it is or isn't possible. If I'm still > unsre, > > is a DCHECK ok for now? > > If you're not sure, and you're hoping to get data to inform things, you might > want a CHECK instead of a DCHECK, so that having a NULL entry will cause an > instant crash. Then you'd want to change this to a DCHECK if it seemed to work > correctly. After looking at the code, and experimenting somewhat, it seems unlikely that SetZoomLevel() ever gets called without a valid navigation entry being available (since the creation of the new tab page in and of itself creates a navigation). I does seem to be the case that GetZoomLevel is called before this, and thus can have null navigation entries. I suppose there could still be cases where SetZoomLevel is called in circumstances I haven't imagined, but let's run this through the try bots and see what happens.
https://codereview.chromium.org/301243018/diff/60001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/301243018/diff/60001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:233: url = entry->GetURL(); Nit: You still have an extra space both here and in the next function.
On 2014/06/04 20:03:39, Peter Kasting wrote: > https://codereview.chromium.org/301243018/diff/60001/content/browser/host_zoo... > File content/browser/host_zoom_map_impl.cc (right): > > https://codereview.chromium.org/301243018/diff/60001/content/browser/host_zoo... > content/browser/host_zoom_map_impl.cc:233: url = entry->GetURL(); > Nit: You still have an extra space both here and in the next function. Sorry, forgot that comment while I was concentrating on the null entry issue. It's fixed, but I'll defer uploading another patch in case anything else needs to change after the try jobs.
On 2014/06/04 20:32:51, wjmaclean wrote: > On 2014/06/04 20:03:39, Peter Kasting wrote: > > > https://codereview.chromium.org/301243018/diff/60001/content/browser/host_zoo... > > File content/browser/host_zoom_map_impl.cc (right): > > > > > https://codereview.chromium.org/301243018/diff/60001/content/browser/host_zoo... > > content/browser/host_zoom_map_impl.cc:233: url = entry->GetURL(); > > Nit: You still have an extra space both here and in the next function. > > Sorry, forgot that comment while I was concentrating on the null entry issue. > It's fixed, but I'll defer uploading another patch in case anything else needs > to change after the try jobs. I've uploaded a patch with the spaces fixed. Non of the try failures look like they're related to the patch (I'm re-running several to see what happens). What do you think, should we try landing this?
On 2014/06/05 13:15:57, wjmaclean wrote: > What do you think, should we try landing this? I don't see why not.
(LGTM)
rubberstamp lgtm
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/301243018/80001
Message was sent while issue was closed.
Change committed as 275268 |
