Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
166633002:
Prefetch @import files from CSS files.
|
Created:
6 years, 10 months ago by jonnyr Modified:
5 years, 9 months ago CC:
blink-reviews, gavinp+loader_chromium.org, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionPrefetch @import files from CSS files.
BUG=
Patch Set 1 #
Total comments: 5
Patch Set 2 : Simplified and with selftests. #
Total comments: 5
Patch Set 3 : Prefetching now handled just like for other resources. #
Total comments: 1
Patch Set 4 : Prefetch @import files from CSS files. Scans until state finished in preload scanner. #
Total comments: 4
Patch Set 5 : Renamed according to comments. #
Total comments: 3
Patch Set 6 : Fixed according to comments. #
Total comments: 1
Patch Set 7 : Use decoded text for scanning. #
Total comments: 2
Patch Set 8 : Use tagname for preloadscanner and preserve request origin like for prefetched css in html style se… #
Total comments: 4
Patch Set 9 : Fixed according to Yoav´s comments #Messages
Total messages: 50 (1 generated)
@reviewer: does this patch look OK to you?
Too tired atm, but happy to review tomorrow.
Don't we have the ability to test the preload scanner?
https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... File Source/core/fetch/CSSStyleSheetResource.h (right): https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... Source/core/fetch/CSSStyleSheetResource.h:69: OwnPtr<HTMLTokenizer> m_tokenizer; This seems wrong. Do other resources own a Tokenizer?
https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... File Source/core/fetch/CSSStyleSheetResource.h (right): https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... Source/core/fetch/CSSStyleSheetResource.h:69: OwnPtr<HTMLTokenizer> m_tokenizer; Why would each resource want its own tokenizer/scanner, etc? Seems like this could be done via a shared, even static method. The whole CSS file is going to be processed in one go.
On 2014/02/14 12:14:53, eseidel wrote: > Don't we have the ability to test the preload scanner? I tested using this: http://stevesouders.com/tests/atimport/link-with-import-big.php The implementations is based on Steve Souders notes in: http://www.stevesouders.com/blog/2014/01/22/browser-wishlist-2013/#lookaheadi... We already have preload scanning of @import in inline CSS in BackgroundHTMLParser.cpp, and I simply reused that. But no, I could not find any testcases or selftests for the preload scanner, at least not in core/html/parser or nearby. I can of course try to add selftests either now or as I improve the preload scanner. Currently @import statements will be ignored and no prefetching done if media rules are included and I would like to fix that as well.
On 2014/02/14 12:15:40, eseidel wrote: > https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... > File Source/core/fetch/CSSStyleSheetResource.h (right): > > https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... > Source/core/fetch/CSSStyleSheetResource.h:69: OwnPtr<HTMLTokenizer> m_tokenizer; > This seems wrong. Do other resources own a Tokenizer? Yes, in BackgroundHTMLParser.cpp the tokenizer is owned. The next step in improving prefetching for CSS files will be to prefetch background images that are used in the HTML based on rule matching. We used to have support for this in Opera and would like to have this ability in Chromium as well. See also comments by Yoav Weiss on http://calendar.perfplanet.com/2013/browser-wishlist-2013/ If done iteratively this requires state, and to get the most out of prefetching we probably prefer not to wait until the full CSS file has been downloaded?
Love the feature. I'm excited to see this happen. This will also be a stepping stone to some other optimizations we have in mind. But I agree with Eric: 1. This isn't factored perfectly yet. Simon Hatch has a similar patch in his client. He might be able to point you at it for some ideas on how to clean this up. 2. We should spend some time thinking about a layout test. Grep around for the existing preload scanner layout tests to see how they work.
On 2014/02/14 13:20:44, jonnyr wrote: > On 2014/02/14 12:14:53, eseidel wrote: > > Don't we have the ability to test the preload scanner? > > I tested using this: > http://stevesouders.com/tests/atimport/link-with-import-big.php > > The implementations is based on Steve Souders notes in: > http://www.stevesouders.com/blog/2014/01/22/browser-wishlist-2013/#lookaheadi... > > We already have preload scanning of @import in inline CSS in > BackgroundHTMLParser.cpp, and I simply reused that. But no, I could not find any > testcases or selftests for the preload scanner, at least not in core/html/parser > or nearby. > > I can of course try to add selftests either now or as I improve the preload > scanner. Currently @import statements will be ignored and no prefetching done if > media rules are included and I would like to fix that as well. You can test the preload scanner by using the internals.isPreloaded method, but its usage is tricky, since you must run it before the element is parsed, otherwise, you'd get false results. You can see a usage example at LayoutTests/http/tests/loading/preload-image-srcset.html
Other than that, I'm excited to see this implemented, and excited to hear that background images are next in line! :)
On 2014/02/14 15:29:59, Yoav Weiss wrote:
> Other than that, I'm excited to see this implemented, and excited to hear that
> background images are next in line! :)
This is getting tangential to this review, so maybe we should start another
thread, but this is something we're considering too. For instance, Simon also
has a patch which allows the preload scanner to pick up any url() within a
body{} rule. That's pretty safe because we know the page will always have a body
even if we haven't done any parsing or style calculating yet. Of course, there
is a very slight chance the rule could get overridden (e.g. with !important),
but that seems reasonably uncommon that it is okay to take the risk of
mis-speculation.
However, speculative matching beyond that could be very risky as it gets
increasingly common that other rules will override. Unmatched url()s in CSS are
extremely common and we don't want to download unused resources. We have some
ideas about how to overcome this, such as keeping a table of element ids seen by
the html tokenizer and then allowing the css preload scanner to request simple
selectors for those ids. But this requires some pretty careful experimentation.
I'm very curious what you have in mind for this space and would love to
brainstorm together on it.
On 2014/02/14 15:41:53, tonyg wrote:
> On 2014/02/14 15:29:59, Yoav Weiss wrote:
> > Other than that, I'm excited to see this implemented, and excited to hear
that
> > background images are next in line! :)
>
> This is getting tangential to this review, so maybe we should start another
> thread, but this is something we're considering too. For instance, Simon also
> has a patch which allows the preload scanner to pick up any url() within a
> body{} rule. That's pretty safe because we know the page will always have a
body
> even if we haven't done any parsing or style calculating yet. Of course, there
> is a very slight chance the rule could get overridden (e.g. with !important),
> but that seems reasonably uncommon that it is okay to take the risk of
> mis-speculation.
>
> However, speculative matching beyond that could be very risky as it gets
> increasingly common that other rules will override. Unmatched url()s in CSS
are
> extremely common and we don't want to download unused resources. We have some
> ideas about how to overcome this, such as keeping a table of element ids seen
by
> the html tokenizer and then allowing the css preload scanner to request simple
> selectors for those ids. But this requires some pretty careful
experimentation.
>
> I'm very curious what you have in mind for this space and would love to
> brainstorm together on it.
Yes, our first implementation of this in Opera gave us this exact problem. We
ended up running a selector match at intervals against the html to see which
images were actually used in the html tree and prefetching only those. Of course
it is possible that js executed later could cause those prefetches to be
invalid, but this did not seem to happen very often. We did not implement
anything to verify that, although that would also be a possibility.
On 2014/02/14 15:21:47, Yoav Weiss wrote: > You can test the preload scanner by using the internals.isPreloaded method, but > its usage is tricky, since you must run it before the element is parsed, > otherwise, you'd get false results. > > You can see a usage example at > LayoutTests/http/tests/loading/preload-image-srcset.html Thank you, very helpful. :)
Happy you're looking into this! Agree with Tony, we should definitely brainstorm some since we've been looking at this area a bit lately as well. https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... File Source/core/fetch/CSSStyleSheetResource.cpp (right): https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... Source/core/fetch/CSSStyleSheetResource.cpp:139: m_preloadScanner->scan(token.data(), m_input->current(), pendingPreloads); What's the advantage to running this through the tokenizer before sending it to the preload scanner? Otherwise, seems like it would be cleaner to just send the data directly.
On 2014/02/14 16:20:17, shatch wrote: > Happy you're looking into this! Agree with Tony, we should definitely brainstorm > some since we've been looking at this area a bit lately as well. > > https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... > File Source/core/fetch/CSSStyleSheetResource.cpp (right): > > https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... > Source/core/fetch/CSSStyleSheetResource.cpp:139: > m_preloadScanner->scan(token.data(), m_input->current(), pendingPreloads); > What's the advantage to running this through the tokenizer before sending it to > the preload scanner? Otherwise, seems like it would be cleaner to just send the > data directly. Agreed, it is probably better to send it directly, since the tokenizer is not really used. I was reusing the existing API to make my change as small as possible.
not lgtm We do want to prefetch @imports from CSS style sheets, but this CL is introducing a bunch of unwanted dependencies from fetch to Document and the HTML parser. We'll need to figure out another way to structure this change. Perhaps the code that fetches the CSSStyleSheetResource should drive the prefetching process instead of trying to teach CSSStyleSheetResource itself about parsing. https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... File Source/core/fetch/CSSStyleSheetResource.h (right): https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... Source/core/fetch/CSSStyleSheetResource.h:34: #include "core/html/parser/HTMLTokenizer.h" Woah there. Fetch shouldn't need to know about any of these detailed things from the parser. https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... Source/core/fetch/CSSStyleSheetResource.h:45: CSSStyleSheetResource(const ResourceRequest&, const String& charset, Document*); Resources shouldn't be specific to documents.
On 2014/02/15 18:48:48, abarth wrote: > not lgtm > > We do want to prefetch @imports from CSS style sheets, but this CL is > introducing a bunch of unwanted dependencies from fetch to Document and the HTML > parser. > > We'll need to figure out another way to structure this change. Perhaps the code > that fetches the CSSStyleSheetResource should drive the prefetching process > instead of trying to teach CSSStyleSheetResource itself about parsing. > > https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... > File Source/core/fetch/CSSStyleSheetResource.h (right): > > https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... > Source/core/fetch/CSSStyleSheetResource.h:34: #include > "core/html/parser/HTMLTokenizer.h" > Woah there. Fetch shouldn't need to know about any of these detailed things > from the parser. Agreed, now it is much simpler and includes a selftest. I hope this helps? > > https://codereview.chromium.org/166633002/diff/1/Source/core/fetch/CSSStyleSh... > Source/core/fetch/CSSStyleSheetResource.h:45: CSSStyleSheetResource(const > ResourceRequest&, const String& charset, Document*); > Resources shouldn't be specific to documents. I have been looking for a solution here, but there is still a dependency on knowing the document for running the prefetch operation. I wonder if this is acceptable, or if there is another way of finding the document so that HTMLResourcePreloader can still be used for prefetching imports?
lg2m, but leaving it up to abarth to provide the approval.
not lgtm You've still got a dependency from a subclass of Resource to a Document. That dependency doesn't make any sense because Resource are shared by many documents. In principle, core/fetch should be in a layer below core and not have any dependencies on core. As I wrote before, rather than putting this logic into the Resource itself, we should put the logic into the code that fetches the resource. https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... File Source/core/fetch/CSSStyleSheetResource.cpp (right): https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... Source/core/fetch/CSSStyleSheetResource.cpp:46: , m_preloader(adoptPtr(new HTMLResourcePreloader(document))) Why do we create these objects in the case where we're not going to use them? They're big complex objects. https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... Source/core/fetch/CSSStyleSheetResource.cpp:47: , m_prescan(document ? true : false) There's no reason to write foo ? true : false. If the compiler can convert foo to a Boolean for the ? : operator, then you can just store it directly. https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... Source/core/fetch/CSSStyleSheetResource.cpp:138: m_preloader->takeAndPreload(pendingPreloads); Whey don't we destroy these objects when we're done with them? https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... File Source/core/fetch/CSSStyleSheetResource.h (right): https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... Source/core/fetch/CSSStyleSheetResource.h:43: CSSStyleSheetResource(const ResourceRequest&, const String& charset, Document*); You can't have a dependency from a subclass of Resource to Document. Resources are shared by multiple documents. https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... Source/core/fetch/CSSStyleSheetResource.h:69: bool m_prescan; What do m_prescan mean? Does this mean m_isPreloadScanning? Can we use the nullity of m_preloadScanner to check for that instead?
On 2014/02/26 19:13:35, abarth wrote: > not lgtm > > You've still got a dependency from a subclass of Resource to a Document. That > dependency doesn't make any sense because Resource are shared by many documents. > In principle, core/fetch should be in a layer below core and not have any > dependencies on core. > > As I wrote before, rather than putting this logic into the Resource itself, we > should put the logic into the code that fetches the resource. > > https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... > File Source/core/fetch/CSSStyleSheetResource.cpp (right): > > https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... > Source/core/fetch/CSSStyleSheetResource.cpp:46: , m_preloader(adoptPtr(new > HTMLResourcePreloader(document))) > Why do we create these objects in the case where we're not going to use them? > They're big complex objects. > > https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... > Source/core/fetch/CSSStyleSheetResource.cpp:47: , m_prescan(document ? true : > false) > There's no reason to write foo ? true : false. If the compiler can convert foo > to a Boolean for the ? : operator, then you can just store it directly. > > https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... > Source/core/fetch/CSSStyleSheetResource.cpp:138: > m_preloader->takeAndPreload(pendingPreloads); > Whey don't we destroy these objects when we're done with them? > > https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... > File Source/core/fetch/CSSStyleSheetResource.h (right): > > https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... > Source/core/fetch/CSSStyleSheetResource.h:43: CSSStyleSheetResource(const > ResourceRequest&, const String& charset, Document*); > You can't have a dependency from a subclass of Resource to Document. Resources > are shared by multiple documents. > > https://codereview.chromium.org/166633002/diff/200001/Source/core/fetch/CSSSt... > Source/core/fetch/CSSStyleSheetResource.h:69: bool m_prescan; > What do m_prescan mean? Does this mean m_isPreloadScanning? Can we use the > nullity of m_preloadScanner to check for that instead? I believe the new design takes everything into consideration, thanks for your reviews and patience.
Thanks, this approach looks a lot better! https://codereview.chromium.org/166633002/diff/220001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/220001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:437: m_isPreloadScanning = false; Why do we only scan the first chunk? What if |length| is 1? It seems better to scan until we've reached some state in the preload scanner..
On 2014/03/05 00:27:35, abarth wrote: > Thanks, this approach looks a lot better! > > https://codereview.chromium.org/166633002/diff/220001/Source/core/html/HTMLLi... > File Source/core/html/HTMLLinkElement.cpp (right): > > https://codereview.chromium.org/166633002/diff/220001/Source/core/html/HTMLLi... > Source/core/html/HTMLLinkElement.cpp:437: m_isPreloadScanning = false; > Why do we only scan the first chunk? What if |length| is 1? It seems better to > scan until we've reached some state in the preload scanner.. Agreed, and fixed.
https://codereview.chromium.org/166633002/diff/240001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/240001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:509: if (m_preloadScanner.get()) { Normally these are written as early returns. if (!m_preloadScannner) return; https://codereview.chromium.org/166633002/diff/240001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:515: if (m_preloadScanner->isDoneParsing()) { I'm not familiar with this method. How does the preload scanner know when the document is done?
On 2014/05/20 06:27:03, eseidel wrote: > https://codereview.chromium.org/166633002/diff/240001/Source/core/html/HTMLLi... > File Source/core/html/HTMLLinkElement.cpp (right): > > https://codereview.chromium.org/166633002/diff/240001/Source/core/html/HTMLLi... > Source/core/html/HTMLLinkElement.cpp:509: if (m_preloadScanner.get()) { > Normally these are written as early returns. > > if (!m_preloadScannner) > return; Thanks, still getting used to code conventions. > > https://codereview.chromium.org/166633002/diff/240001/Source/core/html/HTMLLi... > Source/core/html/HTMLLinkElement.cpp:515: if (m_preloadScanner->isDoneParsing()) > { > I'm not familiar with this method. How does the preload scanner know when the > document is done? The preload scanner holds an internal state, so I added a method in the preload scanner to expose when state reaches DoneParsingImportRules. This state is reached very quickly as soon as all import statements have been processed, since that is the only thing supported by the preload scanner. Basically it is reached whenever the scanner sees a comment or a rule.
but appendData will be called for each packet off the network, right? I see, I guess @imports have to be at the top of a CSS file, so it's OK to stop as soon as we see a rule.
https://codereview.chromium.org/166633002/diff/240001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/240001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:440: , m_input(adoptPtr(new SegmentedString)) You might want to name this m_preloaderInput? Since it's specific to the preloader? https://codereview.chromium.org/166633002/diff/240001/Source/core/html/parser... File Source/core/html/parser/CSSPreloadScanner.h (right): https://codereview.chromium.org/166633002/diff/240001/Source/core/html/parser... Source/core/html/parser/CSSPreloadScanner.h:48: bool isDoneParsing() { return m_state == DoneParsingImportRules; } It's probably better to call this isDoneParsingImports, or something so that callers can have an understanding of when this state is reached. We wouldn't want this to get re-used for something else if the CSSPreloadScanner is taught how to handle more ypes.
lgtm with the renames, thanks.
The CQ bit was checked by jonnyr@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jonnyr@opera.com/166633002/260001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: abarth@chromium.org. Please make sure to get positive review before another CQ attempt.
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: abarth@chromium.org. Please make sure to get positive review before another CQ attempt.
Ptal?
On 2014/05/22 07:41:42, jonnyr wrote: > Ptal? abarth is OOO for a while. eseidel should confirm, but based on the above I would just bypass the recorded disapproval and dcommit.
lgtm https://codereview.chromium.org/166633002/diff/260001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/260001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:518: if (m_preloadScanner->isDoneParsingImports()) { Could you explain the magic here a bit more? // Note: Although dataRecieved will be called for each packet of data returned from the network // @import statements always are at the top of the CSS file. As soon as we reach // a part of the CSS file which is not an @import we can delete our preloader and ignore // the rest of file. If we ever make CSS preloading more complicated // (to look for URLs other than @imports) we will need to re-think this logic.
Thanks! This CL has a much better structure. A few minor comments below. https://codereview.chromium.org/166633002/diff/260001/Source/core/fetch/CSSSt... File Source/core/fetch/CSSStyleSheetResource.cpp (right): https://codereview.chromium.org/166633002/diff/260001/Source/core/fetch/CSSSt... Source/core/fetch/CSSStyleSheetResource.cpp:117: while (StyleSheetResourceClient* c = w.next()) Please use complete words as variable names. w -> walker c -> client https://codereview.chromium.org/166633002/diff/260001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/260001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:513: m_preloadInput->append(SegmentedString(data)); Why don't we need to use |length| on this line?
On 2014/06/26 16:37:59, abarth wrote: > Thanks! This CL has a much better structure. A few minor comments below. > > https://codereview.chromium.org/166633002/diff/260001/Source/core/fetch/CSSSt... > File Source/core/fetch/CSSStyleSheetResource.cpp (right): > > https://codereview.chromium.org/166633002/diff/260001/Source/core/fetch/CSSSt... > Source/core/fetch/CSSStyleSheetResource.cpp:117: while > (StyleSheetResourceClient* c = w.next()) > Please use complete words as variable names. > > w -> walker > c -> client > Done. > https://codereview.chromium.org/166633002/diff/260001/Source/core/html/HTMLLi... > File Source/core/html/HTMLLinkElement.cpp (right): > > https://codereview.chromium.org/166633002/diff/260001/Source/core/html/HTMLLi... > Source/core/html/HTMLLinkElement.cpp:513: > m_preloadInput->append(SegmentedString(data)); > Why don't we need to use |length| on this line? We should, fixed now.
https://codereview.chromium.org/166633002/diff/280001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/280001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:553: m_preloadInput->append(SegmentedString(String(data, length))); How can you take a const char* and make a String out of it? What encoding is |data| in? Don't we need to run it through the appropriate decoder?
On 2014/07/29 16:43:52, abarth wrote: > https://codereview.chromium.org/166633002/diff/280001/Source/core/html/HTMLLi... > File Source/core/html/HTMLLinkElement.cpp (right): > > https://codereview.chromium.org/166633002/diff/280001/Source/core/html/HTMLLi... > Source/core/html/HTMLLinkElement.cpp:553: > m_preloadInput->append(SegmentedString(String(data, length))); > How can you take a const char* and make a String out of it? What encoding is > |data| in? Don't we need to run it through the appropriate decoder? Yes, you are absolutely correct. Thanks, fixed now.
Getting close. One more bug... https://codereview.chromium.org/166633002/diff/300001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/300001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:560: m_preloadLength = sheetText.length(); Why do you call sheetText.substring(m_preloadLength) twice? That's an expensive operation that mallocs a new string buffer and copies the data into the buffer. I don't understand why we need to pass the sheetText substring in twice, once in m_preloadInput and once as the first argument to m_preloadScanner->scan. In fact, this is wrong: void CSSPreloadScanner::scan(const String& tagName, const SegmentedString& source, PreloadRequestStream& requests) The first argument is the tagName, not the CSS source.
tonyg@chromium.org changed reviewers: - tonyg@chromium.org
abarth, could you ptal? https://codereview.chromium.org/166633002/diff/300001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/300001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:560: m_preloadLength = sheetText.length(); On 2014/07/31 14:54:23, abarth wrote: > Why do you call sheetText.substring(m_preloadLength) twice? That's an expensive > operation that mallocs a new string buffer and copies the data into the buffer. > > I don't understand why we need to pass the sheetText substring in twice, once in > m_preloadInput and once as the first argument to m_preloadScanner->scan. In > fact, this is wrong: > > void CSSPreloadScanner::scan(const String& tagName, const SegmentedString& > source, PreloadRequestStream& requests) > > The first argument is the tagName, not the CSS source. First of all sorry for the delay in following up on this, I am back from five months on parental leave. I rewrote this section and started using the html tokenizer so that tagName is passed to CSSPreloadScanner::scan(), just like it is done in the html background parser. The main reason was to reuse the tokenizer so that the second parameter, source, can keep track of request origin. This is used in Developer Tools to see initiator on the network tab.
A couple of drive-by nits. https://codereview.chromium.org/166633002/diff/320001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/320001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:567: if (!m_preloadScanner) Maybe a comment saying that the absence of m_preloadScanner means that we passed the "@import" phase of that resource? https://codereview.chromium.org/166633002/diff/320001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:578: Why not merge the if condition into the while loop? Something like: while (m_tokenizer->nextToken(m_input.current(), *m_token)) { Compact..... }
Thanks for the comments Yoav. https://codereview.chromium.org/166633002/diff/320001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/166633002/diff/320001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:567: if (!m_preloadScanner) On 2015/01/05 13:18:44, Yoav Weiss wrote: > Maybe a comment saying that the absence of m_preloadScanner means that we passed > the "@import" phase of that resource? Done. https://codereview.chromium.org/166633002/diff/320001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:578: On 2015/01/05 13:18:44, Yoav Weiss wrote: > Why not merge the if condition into the while loop? > > Something like: > while (m_tokenizer->nextToken(m_input.current(), *m_token)) { > Compact..... > > > } Done.
ping?
On 2015/01/19 10:51:04, jonnyr wrote: > ping? I think you need abarth's OK here, since he previously did not approve.
On 2015/01/19 11:49:15, Yoav Weiss wrote: > On 2015/01/19 10:51:04, jonnyr wrote: > > ping? > > I think you need abarth's OK here, since he previously did not approve. Yes, but how do I get abarth to take a look again? Send him a mail privately?
On 2015/01/19 12:03:40, jonnyr wrote: > On 2015/01/19 11:49:15, Yoav Weiss wrote: > > On 2015/01/19 10:51:04, jonnyr wrote: > > > ping? > > > > I think you need abarth's OK here, since he previously did not approve. > > Yes, but how do I get abarth to take a look again? Send him a mail privately? Yeah, try that.
On 2015/01/19 at 14:22:52, yoav wrote: > On 2015/01/19 12:03:40, jonnyr wrote: > > On 2015/01/19 11:49:15, Yoav Weiss wrote: > > > On 2015/01/19 10:51:04, jonnyr wrote: > > > > ping? > > > > > > I think you need abarth's OK here, since he previously did not approve. > > > > Yes, but how do I get abarth to take a look again? Send him a mail privately? > > Yeah, try that. Sorry for not noticing this CL. Probably the best thing is to open up a new CL that doesn't have the red flag attached to it. Sorry for the extra hassle. :( |
