Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196743002:
Parse the link element's size attribute (Closed)
|
Created:
6 years, 9 months ago by michaelbai Modified:
6 years, 9 months ago CC:
blink-reviews, jamesr, gavinp+prerender_chromium.org, sof, eae+blinkwatch, abarth-chromium, dglazkov+blink, adamk+blink_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionThis patch parse the sizes attribute into vector of IntSize, also added sizes in WebIconURL, so it could be used by chromium
BUG=349039
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169404
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : #Patch Set 4 : use charactersToInt #
Total comments: 6
Patch Set 5 : address comments #
Total comments: 2
Patch Set 6 : #
Total comments: 3
Patch Set 7 : fix the space #
Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:65: RefPtr<DOMSettableTokenList> tokenList = DOMSettableTokenList::create(); Why are we using a DOMSettableTokenList here? https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:69: for (unsigned i = 0; i < tokens.size(); i++) { ++i https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:70: const String& sizeString = (String&)tokens[i]; Why is (String&) needed here? We generally don't use C-style casts, and I'm not sure why we'd need a cast at all. https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:93: } This function is very slow. Why not write a version that just walks the string and parses it? https://codereview.chromium.org/196743002/diff/60001/public/web/WebIconURL.h File public/web/WebIconURL.h (right): https://codereview.chromium.org/196743002/diff/60001/public/web/WebIconURL.h#... public/web/WebIconURL.h:76: } You're missing a blank line after this line.
https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:55: const String sizeSeparator = "x"; This could be a UChar. https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:63: void HTMLLinkElement::parseSizesAttribute(const AtomicString& value, Vector<IntSize>* iconSizes) The Vector could be passed by reference instead of pointer. https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:64: { Should we add an assertion to make sure the Vector is initially empty?
PTAL https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:63: void HTMLLinkElement::parseSizesAttribute(const AtomicString& value, Vector<IntSize>* iconSizes) iconSizes will change and should be pass as pointer, right? On 2014/03/12 20:09:52, Chris Dumez wrote: > The Vector could be passed by reference instead of pointer. https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:64: { On 2014/03/12 20:09:52, Chris Dumez wrote: > Should we add an assertion to make sure the Vector is initially empty? Done. https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:93: } I were thinking I should use existing utility, changed to parse it. On 2014/03/12 19:42:34, abarth wrote: > This function is very slow. Why not write a version that just walks the string > and parses it? https://codereview.chromium.org/196743002/diff/60001/public/web/WebIconURL.h File public/web/WebIconURL.h (right): https://codereview.chromium.org/196743002/diff/60001/public/web/WebIconURL.h#... public/web/WebIconURL.h:76: } On 2014/03/12 19:42:34, abarth wrote: > You're missing a blank line after this line. Done.
https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:63: void HTMLLinkElement::parseSizesAttribute(const AtomicString& value, Vector<IntSize>* iconSizes) On 2014/03/12 23:08:41, michaelbai wrote: > iconSizes will change and should be pass as pointer, right? > > On 2014/03/12 20:09:52, Chris Dumez wrote: > > The Vector could be passed by reference instead of pointer. > I said a by reference, not by *const* reference.
On 2014/03/12 23:14:37, Chris Dumez wrote: > https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... > File Source/core/html/HTMLLinkElement.cpp (right): > > https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... > Source/core/html/HTMLLinkElement.cpp:63: void > HTMLLinkElement::parseSizesAttribute(const AtomicString& value, Vector<IntSize>* > iconSizes) > On 2014/03/12 23:08:41, michaelbai wrote: > > iconSizes will change and should be pass as pointer, right? > > > > On 2014/03/12 20:09:52, Chris Dumez wrote: > > > The Vector could be passed by reference instead of pointer. > > > > I said a by reference, not by *const* reference. The reason I prefer a reference is because the implementation is expecting the pointer to be non-null (even though there is no assertion in place to make sure of this).
https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:64: ASSERT(!iconSizes->size()); ASSERT(iconSizes.isEmpty()); would be more readable. https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:65: if (value.isNull() || value.isEmpty()) if (value.isEmpty()) The isNull() check is superfluous as it is already included in your isEmpty() check. https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:74: StringBuffer<UChar> buff(value.length()); We prefer not to use abbreviations. Why do we need a StringBuffer at all. Can't we just track the start and the end with pointers in the original String and then use something like WTF::charactersToInt(start, end - start)? Adam probably has a good proposal here. :) Right now, having a StringBuffer + constructing temporary Strings to convert to integer seems very inefficient to me. https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:75: unsigned buffIndex = 0; Ditto
https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/60001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:63: void HTMLLinkElement::parseSizesAttribute(const AtomicString& value, Vector<IntSize>* iconSizes) I has been told to use pointer in this case, I searched blink-dev, it seemed we decide to use reference here. On 2014/03/12 23:14:38, Chris Dumez wrote: > On 2014/03/12 23:08:41, michaelbai wrote: > > iconSizes will change and should be pass as pointer, right? > > > > On 2014/03/12 20:09:52, Chris Dumez wrote: > > > The Vector could be passed by reference instead of pointer. > > > > I said a by reference, not by *const* reference. https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:64: ASSERT(!iconSizes->size()); On 2014/03/12 23:27:13, Chris Dumez wrote: > ASSERT(iconSizes.isEmpty()); would be more readable. Done. https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:65: if (value.isNull() || value.isEmpty()) On 2014/03/12 23:27:13, Chris Dumez wrote: > if (value.isEmpty()) > > The isNull() check is superfluous as it is already included in your isEmpty() > check. Done. https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:74: StringBuffer<UChar> buff(value.length()); Can we assume the passed in |value| is always 8bit? otherwise, WTF::charactersToInt probably can't use it here because we don't know AtomicString::characters8() or AtomicString::characters16() should used. On 2014/03/12 23:27:13, Chris Dumez wrote: > We prefer not to use abbreviations. Why do we need a StringBuffer at all. Can't > we just track the start and the end with pointers in the original String and > then use something like WTF::charactersToInt(start, end - start)? > > Adam probably has a good proposal here. :) Right now, having a StringBuffer + > constructing temporary Strings to convert to integer seems very inefficient to > me.
ping
https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/80001/Source/core/html/HTMLLin... Source/core/html/HTMLLinkElement.cpp:74: StringBuffer<UChar> buff(value.length()); On 2014/03/13 05:50:11, michaelbai wrote: > Can we assume the passed in |value| is always 8bit? otherwise, > WTF::charactersToInt probably can't use it here because we don't know > AtomicString::characters8() or AtomicString::characters16() should used. I don't think you can make this assumption although I am not 100% sure. I believe you need to handle both cases 8bit and 16bit. You can use CSSMarkup.cpp as reference (it does something very similar to what you're doing and avoids duplication between 8/16 bits cases using templates).
PTAL
https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:56: static void parseSizes(const CharacterType value, unsigned length, Vector<IntSize>& iconSizes) const CharacterType* ? https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:68: while (i < length) { I think a for loop would be slightly better here since you have a start and end index. https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElementSizesAttributeTest.cpp (right): https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElementSizesAttributeTest.cpp:2: * Copyright 2014, Google Inc. All rights reserved. Please use new license HEADER: http://www.chromium.org/blink/coding-style#TOC-License
Thanks, PTAL https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:56: static void parseSizes(const CharacterType value, unsigned length, Vector<IntSize>& iconSizes) On 2014/03/14 20:36:18, Chris Dumez wrote: > const CharacterType* ? Done. https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:68: while (i < length) { On 2014/03/14 20:36:18, Chris Dumez wrote: > I think a for loop would be slightly better here since you have a start and end > index. Done. https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElementSizesAttributeTest.cpp (right): https://codereview.chromium.org/196743002/diff/120001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElementSizesAttributeTest.cpp:2: * Copyright 2014, Google Inc. All rights reserved. On 2014/03/14 20:36:18, Chris Dumez wrote: > Please use new license HEADER: > http://www.chromium.org/blink/coding-style#TOC-License Done.
Thanks for taking the feedback into consideration. I know better than to review string parsing code so I will let Adam take a look now. https://codereview.chromium.org/196743002/diff/140001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/140001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:68: for (;i < length; ++i) { "; i" <- missing space
Thanks for your time to help review this! @abarth, PTAL https://codereview.chromium.org/196743002/diff/140001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/140001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:68: for (;i < length; ++i) { On 2014/03/14 20:53:51, Chris Dumez wrote: > "; i" <- missing space Done.
LGTM https://codereview.chromium.org/196743002/diff/160001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/160001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:68: for ( ;i < length; ++i) { We usually put a space after ;
https://codereview.chromium.org/196743002/diff/160001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/160001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:68: for ( ;i < length; ++i) { On 2014/03/17 18:15:50, abarth wrote: > We usually put a space after ; ahah, I complained about this on the last patch iteration but it seems the space was added before the ; instead of after... :)
Thanks https://codereview.chromium.org/196743002/diff/160001/Source/core/html/HTMLLi... File Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/196743002/diff/160001/Source/core/html/HTMLLi... Source/core/html/HTMLLinkElement.cpp:68: for ( ;i < length; ++i) { Oh, right, it should be after; On 2014/03/17 18:17:49, Chris Dumez wrote: > On 2014/03/17 18:15:50, abarth wrote: > > We usually put a space after ; > > ahah, I complained about this on the last patch iteration but it seems the space > was added before the ; instead of after... :)
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/196743002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on mac_blink_rel
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/196743002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_rel
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/196743002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/196743002/180001
Message was sent while issue was closed.
Change committed as 169404 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
