Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1377163002:
Use DOMSettableTokenList for {HTMLAnchorElement, HTMLAreaElement}.ping. (Closed)
|
Created:
5 years, 2 months ago by hyunjunekim2 Modified:
5 years ago CC:
blink-reviews, vivekg_samsung, blink-reviews-html_chromium.org, vivekg, dglazkov+blink, Inactive Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse DOMSettableTokenList for {HTMLAnchorElement, HTMLAreaElement}.ping.
This CL is that replace DOMString with DOMSettableTokenList
for {HTMLAnchorElement, HTMLAreaElement}.ping.
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/Z-RBeFLHRSU/0EY_qBmdtegJ
BUG=498219
Committed: https://crrev.com/faf57a0c2dceb272f8823251efd08fa4dceb72d0
Cr-Commit-Position: refs/heads/master@{#353261}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Lazy #Patch Set 3 : update test-case. #Patch Set 4 : Lazy2 #Patch Set 5 : test #Patch Set 6 : test2 #Patch Set 7 : Lazy3 #
Total comments: 6
Patch Set 8 : Lazy4 #Patch Set 9 : Lazy5 #Patch Set 10 : #
Total comments: 3
Patch Set 11 : #Patch Set 12 : fixup presubmit. #Patch Set 13 : fixup presubmit2 #Patch Set 14 : #Patch Set 15 #Patch Set 16 : warning C4275 #
Total comments: 1
Patch Set 17 : build on win1 #Patch Set 18 : build on win2 #Patch Set 19 : public -> private #Patch Set 20 #Patch Set 21 : Original #Patch Set 22 : Remove CORE_EXPORT on {HTMLAnchor, HTMLArea} #Patch Set 23 : Add CORE_EXPORT on DOMSettableTokenListObserver #
Total comments: 1
Patch Set 24 : #Patch Set 25 : Rebase #
Messages
Total messages: 49 (19 generated)
hyunjune.kim@samsung.com changed reviewers: + mkwst@chromium.org, pdr@chromium.org, philipj@opera.com, tkent@chromium.org
Could you check this patch? I created new issue.
https://codereview.chromium.org/1377163002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/ping-attribute-dom-binding.html (right): https://codereview.chromium.org/1377163002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/ping-attribute-dom-binding.html:17: anchor.ping.add('p3'); If there isn't already another test doing it, also try adding a token that is already present and verify that it does nothing. https://codereview.chromium.org/1377163002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/1377163002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:53: , m_ping(DOMSettableTokenList::create(this)) This should mean a new heap-allocated object for every <a> in the world, and there are a lot of those. I see that e.g. HTMLLinkElement does it like this, but could this be lazily created in the ping() getter so that the price is only payed for the few cases where the ping attribute is used?
philipj, Could you check PS7?
https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:236: m_ping = DOMSettableTokenList::create((DOMSettableTokenListObserver*)this); It looks like you could use ping()->setValue(value) here to achieve the same thing. https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:351: m_ping = DOMSettableTokenList::create((DOMSettableTokenListObserver*)this); Why is the case needed? If it is needed, it should be a static_cast.
https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:236: m_ping = DOMSettableTokenList::create((DOMSettableTokenListObserver*)this); On 2015/10/05 15:03:17, philipj wrote: > It looks like you could use ping()->setValue(value) here to achieve the same > thing. When call |anchor.setAttribute('ping', 'p1');|, if hasn't this codes, occur to segment fault. https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:351: m_ping = DOMSettableTokenList::create((DOMSettableTokenListObserver*)this); On 2015/10/05 15:03:17, philipj wrote: > Why is the case needed? currently lazily it did make. If there is'n this code, return undefined then call |a.ping|. >If it is needed, it should be a static_cast. |this| object is const, So don't use a static_cast.
https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:236: m_ping = DOMSettableTokenList::create((DOMSettableTokenListObserver*)this); On 2015/10/05 15:30:25, hyunjunekim2 wrote: > On 2015/10/05 15:03:17, philipj wrote: > > It looks like you could use ping()->setValue(value) here to achieve the same > > thing. > When call |anchor.setAttribute('ping', 'p1');|, if hasn't this codes, occur to > segment fault. It will segfault if you use use m_ping->, but why does ping()-> not work? https://codereview.chromium.org/1377163002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:351: m_ping = DOMSettableTokenList::create((DOMSettableTokenListObserver*)this); On 2015/10/05 15:30:25, hyunjunekim2 wrote: > On 2015/10/05 15:03:17, philipj wrote: > > Why is the case needed? > currently lazily it did make. If there is'n this code, return undefined then > call |a.ping|. > > >If it is needed, it should be a static_cast. > |this| object is const, So don't use a static_cast. It seems like just making ping() not const would be better, but there's also const_cast of course.
philipj, Could you check PS10?
philipj@opera.com changed reviewers: + sigbjornf@opera.com
lgtm % mutable nit sof, can you check this for Oilpan sanity? https://codereview.chromium.org/1377163002/diff/170009/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.h (right): https://codereview.chromium.org/1377163002/diff/170009/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.h:124: mutable RefPtrWillBeMember<DOMSettableTokenList> m_ping; m_ping no longer needs to be mutable.
oilpan lgtm. https://codereview.chromium.org/1377163002/diff/170009/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/1377163002/diff/170009/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:398: setSynchronizedLazyAttribute(pingAttr, m_ping->value()); m_ping is guaranteed to be non-null?
https://codereview.chromium.org/1377163002/diff/170009/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/1377163002/diff/170009/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:398: setSynchronizedLazyAttribute(pingAttr, m_ping->value()); On 2015/10/06 08:16:15, sof wrote: > m_ping is guaranteed to be non-null? I think so, because valueChanged() is called from DOMSettableTokenList::setValue() and the lifetimes of the two objects look right.
philipj, sof, Could you check PS11?
The CQ bit was checked by hyunjune.kim@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1377163002/#ps190001 (title: " ")
The CQ bit was unchecked by hyunjune.kim@samsung.com
The CQ bit was checked by philipj@opera.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1377163002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1377163002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hyunjune.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1377163002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1377163002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hyunjune.kim@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1377163002/#ps210001 (title: "fixup presubmit.")
The CQ bit was unchecked by hyunjune.kim@samsung.com
The CQ bit was checked by hyunjune.kim@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1377163002/#ps230001 (title: "fixup presubmit2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1377163002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1377163002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/1377163002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.h (right): https://codereview.chromium.org/1377163002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.h:61: #pragma warning(disable: 4275) // Disable warning C4275 This is done very rarely in Blink. What is the warning?
On 2015/10/07 13:02:38, philipj wrote: > https://codereview.chromium.org/1377163002/diff/290001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLAnchorElement.h (right): > > https://codereview.chromium.org/1377163002/diff/290001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLAnchorElement.h:61: #pragma > warning(disable: 4275) // Disable warning C4275 > This is done very rarely in Blink. What is the warning? I am checking it.
philipj, warning C4275 - https://msdn.microsoft.com/en-us/library/vstudio/3tdb471s(v=vs.100).aspx - https://msdn.microsoft.com/en-us/library/3tdb471s.aspx - http://stackoverflow.com/questions/5207765/c4275-warning-in-visual-studio I guess it's dll problem. if class on dll inherit external class, i think occur to this warning. And I fixed up by adding |CORE_EXPORT|. Could you re-check PS18?
On 2015/10/07 14:47:17, hyunjunekim2 wrote: > philipj, > warning C4275 > - https://msdn.microsoft.com/en-us/library/vstudio/3tdb471s(v=vs.100).aspx > - https://msdn.microsoft.com/en-us/library/3tdb471s.aspx > - http://stackoverflow.com/questions/5207765/c4275-warning-in-visual-studio > > I guess it's dll problem. if class on dll inherit external class, i think occur > to this warning. > > And I fixed up by adding |CORE_EXPORT|. Could you re-check PS18? Is this just because of HTMLAnchorElement now inherits from DOMSettableTokenListObserver? If so, does it help to make that inheritance private? This all seems odd because this isn't the first HTML element with a DOMSettableTokenList member, so which is the relevant difference?
On 2015/10/07 15:10:02, philipj wrote: > On 2015/10/07 14:47:17, hyunjunekim2 wrote: > > philipj, > > warning C4275 > > - https://msdn.microsoft.com/en-us/library/vstudio/3tdb471s(v=vs.100).aspx > > - https://msdn.microsoft.com/en-us/library/3tdb471s.aspx > > - http://stackoverflow.com/questions/5207765/c4275-warning-in-visual-studio > > > > I guess it's dll problem. if class on dll inherit external class, i think > occur > > to this warning. > > > > And I fixed up by adding |CORE_EXPORT|. Could you re-check PS18? > > Is this just because of HTMLAnchorElement now inherits from > DOMSettableTokenListObserver? If so, does it help to make that inheritance > private? > > This all seems odd because this isn't the first HTML element with a > DOMSettableTokenList member, so which is the relevant difference? On PS19, i replaced public with private on HTMLAnchorElement.h I added |CORE_EXPORT| between class keyword and class's name on DOMTokenList.h and DOMSettableTokenList.h. and DOMTokenList class inherits ScriptWrappable, Also ScriptWrappable has |CORE_EXPORT|.
philipj, Could you check PS21 and PS23? About https://code.google.com/p/chromium/issues/detail?id=358074, If add |CORE_EXPORT|, I guess it's to make webcore.dll.
hyunjune.kim@samsung.com changed reviewers: + haraken@chromium.org
lgtm with nit. The other class inheriting from DOMSettableTokenListObserver is HTMLIFrameElement does not have CORE_EXPORT, but HTMLAnchorelement does, so that's why DOMSettableTokenListObserver must have it now. https://codereview.chromium.org/1377163002/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.h (right): https://codereview.chromium.org/1377163002/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.h:60: class CORE_EXPORT HTMLAnchorElement : public HTMLElement, public DOMURLUtils, private DOMSettableTokenListObserver { Since making it private wasn't the solution, I suggest making this private again to match HTMLIFrameElement.
philipj, Could you re-check PS24? Thank you.
The CQ bit was checked by philipj@opera.com
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1377163002/#ps440001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1377163002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1377163002/440001
Message was sent while issue was closed.
Committed patchset #24 (id:440001)
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/faf57a0c2dceb272f8823251efd08fa4dceb72d0 Cr-Commit-Position: refs/heads/master@{#353261}
Message was sent while issue was closed.
Description was changed from
==========
Use DOMSettableTokenList for {HTMLAnchorElement, HTMLAreaElement}.ping.
This CL is that replace DOMString with DOMSettableTokenList
for {HTMLAnchorElement, HTMLAreaElement}.ping.
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/Z-RBeFLHRSU/0E...
BUG=498219
Committed: https://crrev.com/faf57a0c2dceb272f8823251efd08fa4dceb72d0
Cr-Commit-Position: refs/heads/master@{#353261}
==========
to
==========
Use DOMSettableTokenList for {HTMLAnchorElement, HTMLAreaElement}.ping.
This CL is that replace DOMString with DOMSettableTokenList
for {HTMLAnchorElement, HTMLAreaElement}.ping.
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/Z-RBeFLHRSU/0E...
BUG=498219
Committed: https://crrev.com/faf57a0c2dceb272f8823251efd08fa4dceb72d0
Cr-Commit-Position: refs/heads/master@{#353261}
==========
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
