Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2378323003:
Add url::Origin::GetURL() to convert Origins to URLs without reparsing (Closed)
|
Created:
4 years, 2 months ago by Charlie Harrison Modified:
4 years, 2 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, fuzzing_chromium.org, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd url::Origin::GetURL() to convert Origins to URLs without reparsing
The previous canonical way to convert an Origin to a GURL was
to call GURL(origin.Serialize()). This is expensive and often in the
critical path of user perceived page loads. One such caller has
been converted to the new method, and the rest will be converted
in a followup patch.
BUG=651554
Committed: https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d
Cr-Commit-Position: refs/heads/master@{#422613}
Patch Set 1 #Patch Set 2 : remove fuzzer #
Total comments: 2
Patch Set 3 : s/GURL("null")/GURL() #
Total comments: 8
Patch Set 4 : file:// -> file:/// and constructing Parsed alongside serialization #Patch Set 5 : propagate file:/// change to weborigin #
Total comments: 1
Patch Set 6 : revert the file:/// changes #
Dependent Patchsets: Messages
Total messages: 52 (33 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add url::Origin::GetURL() to convert Origins to URLs without reparsing BUG= ========== to ========== Add url::Origin::GetURL() to convert Origins to URLs without reparsing The previous canonical way to convert an Origin to a GURL was to call GURL(origin.Serialize()). This is expensive and often in the critical path of user perceived page loads. One such caller has been converted to the new method, and the rest will be converted in a followup patch. BUG=651554 ==========
csharrison@chromium.org changed reviewers: + mkwst@chromium.org
Mike, can you take a first pass at this? Thanks!
Actually, in addition to the fuzzer I think I will write another test suite... Feel free to take a pass at the non test code if you care to.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I updated the existing tests to assert that this method performs correctly, and removed the fuzzer which was overkill for this CL. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mkwst@chromium.org changed reviewers: + brettw@chromium.org
Tagging in Brett, because he knows more about GURL parsing and efficiency than I do (and I'm OOO today and Monday :) ). https://codereview.chromium.org/2378323003/diff/20001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/2378323003/diff/20001/url/origin.cc#newcode70 url/origin.cc:70: return GURL("null"); Hrm. I think `GURL::GetOrigin()` returns an empty GURL for an invalid URL. Perhaps it makes sense to return `GURL()` instead?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + creis@chromium.org
Thanks! Also +creis for minor change to render_frame_impl as the first consumer of this new API (which is in the critical path of resource loading). https://codereview.chromium.org/2378323003/diff/20001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/2378323003/diff/20001/url/origin.cc#newcode70 url/origin.cc:70: return GURL("null"); On 2016/09/30 12:37:40, Mike West (OOO until the 4th) wrote: > Hrm. I think `GURL::GetOrigin()` returns an empty GURL for an invalid URL. > Perhaps it makes sense to return `GURL()` instead? GURL("null") == GURL() so that's fine. I would like to maintain that GURL(origin.Serialize()) == origin.GetURL(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2378323003/diff/40001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/2378323003/diff/40001/url/origin.cc#newcode73 url/origin.cc:73: return GURL("file://"); 1. We should have a constant for this so we don't duplicate it here and above. 2. I think we want to pick something else. GURL will canonicalize this to "file:///" which means the root directory (the way to read that is it's got "://" like http does, then a path of "/"). If we want the root directory, then we should probably change both of these to "file:///". https://codereview.chromium.org/2378323003/diff/40001/url/scheme_host_port.cc File url/scheme_host_port.cc (right): https://codereview.chromium.org/2378323003/diff/40001/url/scheme_host_port.cc... url/scheme_host_port.cc:172: std::string serialized = Serialize(); I'm a little nervous about how this depends on the internals of how Serialize works but is separate from it. What about making a new private internal serialize function that returns a string and has a url::Parsed structure as an out param. Then have Serialize call into that and discard the Parsed structure (I wouldn't make this optional since I think null checking will be slower and uglier than just filling it in) and GetURL makes a GURL out of it. https://codereview.chromium.org/2378323003/diff/40001/url/scheme_host_port.cc... url/scheme_host_port.cc:176: static const int schemeSeparatorLength = strlen(kStandardSchemeSeparator); style nit: use underscores and not camelCase (although I think if you take the previous comment's advice this can be deleted). https://codereview.chromium.org/2378323003/diff/40001/url/scheme_host_port.cc... url/scheme_host_port.cc:193: return GURL(serialized.data(), serialized.length(), parsed, true); I told you to use this constructor, but I realize now there's a better one. Do: return GURL(std::move(serialized), parsed, true); which will prevent the allocation of a new string.
creis@chromium.org changed reviewers: + nick@chromium.org
Adding Nick, who's been thinking a lot about this and sounds excited. :)
And yes, render_frame_impl.cc LGTM.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm; this is very useful
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks like lots of code in blink expects file origins to be "file://" not "file:///" (including probably code in the wild) I'll defer to mkwst for if we want to change things there, but I'm partial to just keeping this as "file://" for now. https://codereview.chromium.org/2378323003/diff/40001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/2378323003/diff/40001/url/origin.cc#newcode73 url/origin.cc:73: return GURL("file://"); On 2016/09/30 20:18:48, brettw (ping on IM after 24h) wrote: > 1. We should have a constant for this so we don't duplicate it here and above. > > 2. I think we want to pick something else. GURL will canonicalize this to > "file:///" which means the root directory (the way to read that is it's got > "://" like http does, then a path of "/"). If we want the root directory, then > we should probably change both of these to "file:///". Done. https://codereview.chromium.org/2378323003/diff/40001/url/scheme_host_port.cc File url/scheme_host_port.cc (right): https://codereview.chromium.org/2378323003/diff/40001/url/scheme_host_port.cc... url/scheme_host_port.cc:172: std::string serialized = Serialize(); On 2016/09/30 20:18:48, brettw (ping on IM after 24h) wrote: > I'm a little nervous about how this depends on the internals of how Serialize > works but is separate from it. > > What about making a new private internal serialize function that returns a > string and has a url::Parsed structure as an out param. Then have Serialize call > into that and discard the Parsed structure (I wouldn't make this optional since > I think null checking will be slower and uglier than just filling it in) and > GetURL makes a GURL out of it. Great idea. Done. The only problem I can see is that appending the "/" is different between the two versions. RFC 6454 forbids adding this "/" so I had to special case the GetURL() case. https://codereview.chromium.org/2378323003/diff/40001/url/scheme_host_port.cc... url/scheme_host_port.cc:176: static const int schemeSeparatorLength = strlen(kStandardSchemeSeparator); On 2016/09/30 20:18:48, brettw (ping on IM after 24h) wrote: > style nit: use underscores and not camelCase (although I think if you take the > previous comment's advice this can be deleted). oops, been working too long in Blink :P https://codereview.chromium.org/2378323003/diff/40001/url/scheme_host_port.cc... url/scheme_host_port.cc:193: return GURL(serialized.data(), serialized.length(), parsed, true); On 2016/09/30 20:18:48, brettw (ping on IM after 24h) wrote: > I told you to use this constructor, but I realize now there's a better one. Do: > return GURL(std::move(serialized), parsed, true); > which will prevent the allocation of a new string. :) was hoping there was a better way here. Done!
On 2016/10/03 17:28:48, Charlie Harrison wrote: > Looks like lots of code in blink expects file origins to be "file://" not > "file:///" (including probably code in the wild) I'll defer to mkwst for if we > want to change things there, but I'm partial to just keeping this as "file://" > for now. Yikes. It seems pretty scary that getting the origin as a URL will follow different rules than getting the origin as a string. Can you at least put a discussion of how the returned URL won't follow the RFC for origin serialization above the getter in the header?
https://codereview.chromium.org/2378323003/diff/80001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/2378323003/diff/80001/url/origin.cc#newcode69 url/origin.cc:69: return kRootFileUrl; It looks like this returns the new "file:///" instead which doesn't match what you said in the last review comment. (If these two cases have to be different, the constant at the top probably isn't helping). Assumeing they should be different, it's probably a good idea to call out in comments here in the implementation why (even though we're adding something to the header) so somebody doesn't try to "fix" it later.
Sorry, I haven't changed the CL back to using "file://" for origin serialization, my last comment was just explaining the trybot failures. I'll update an amended PS removing some of these changes and add comments explaining this oddity.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
So, I tried to explain this in the comments but I think the oddity of the file:/// urls are explained by the fact that GURL will always add a "/" path to serialized origins (including ones with valid SchemeHostPorts). So: http://example.test => http://example.test/ file:// => file:///
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks! I'll land now since the changes to weborigin went away.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, nick@chromium.org Link to the patchset: https://codereview.chromium.org/2378323003/#ps100001 (title: "revert the file:/// changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add url::Origin::GetURL() to convert Origins to URLs without reparsing The previous canonical way to convert an Origin to a GURL was to call GURL(origin.Serialize()). This is expensive and often in the critical path of user perceived page loads. One such caller has been converted to the new method, and the rest will be converted in a followup patch. BUG=651554 ========== to ========== Add url::Origin::GetURL() to convert Origins to URLs without reparsing The previous canonical way to convert an Origin to a GURL was to call GURL(origin.Serialize()). This is expensive and often in the critical path of user perceived page loads. One such caller has been converted to the new method, and the rest will be converted in a followup patch. BUG=651554 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add url::Origin::GetURL() to convert Origins to URLs without reparsing The previous canonical way to convert an Origin to a GURL was to call GURL(origin.Serialize()). This is expensive and often in the critical path of user perceived page loads. One such caller has been converted to the new method, and the rest will be converted in a followup patch. BUG=651554 ========== to ========== Add url::Origin::GetURL() to convert Origins to URLs without reparsing The previous canonical way to convert an Origin to a GURL was to call GURL(origin.Serialize()). This is expensive and often in the critical path of user perceived page loads. One such caller has been converted to the new method, and the rest will be converted in a followup patch. BUG=651554 Committed: https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d Cr-Commit-Position: refs/heads/master@{#422613} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/048bee100cbbc999219901a759fc86abc67c726d Cr-Commit-Position: refs/heads/master@{#422613} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
