Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
283663006:
Oilpan: Prepare moving HTMLTemplateElement to oipan (Closed)
|
Created:
6 years, 7 months ago by keishi Modified:
6 years, 7 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionOilpan: Prepare moving HTMLTemplateElement to oipan
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174349
Patch Set 1 #Patch Set 2 : #
Total comments: 13
Patch Set 3 : Fixed #Patch Set 4 : Changed back to pointer #
Messages
Total messages: 26 (0 generated)
LGTM https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... File Source/core/dom/TemplateContentDocumentFragment.cpp (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. You could use the short copyright header for this new file. https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... File Source/core/dom/TemplateContentDocumentFragment.h (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.h:36: static PassRefPtrWillBeRawPtr<TemplateContentDocumentFragment> create(Document& document, Element& host) Why was this changed to a reference; it is used as a pointer in all of this code. https://codereview.chromium.org/283663006/diff/20001/Source/core/html/HTMLTem... File Source/core/html/HTMLTemplateElement.cpp (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/html/HTMLTem... Source/core/html/HTMLTemplateElement.cpp:64: m_content = TemplateContentDocumentFragment::create(document().ensureTemplateDocument(), *const_cast<HTMLTemplateElement*>(this)); Let's just keep it a pointer?
LGTM with comments. https://codereview.chromium.org/283663006/diff/20001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/core.gypi#ne... Source/core/core.gypi:2113: 'dom/TemplateContentDocumentFragment.h', I wonder where TemplateContentDocumentFragment.h was written before. https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... File Source/core/dom/TemplateContentDocumentFragment.cpp (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.cpp:29: */ You can use the new, 4-line copyright. https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.cpp:38: void TemplateContentDocumentFragment::trace(Visitor* visitor) We normally don't add a new file just for a trace() method. If we don't have a cpp file, we can just define the trace() method in a header file. https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... File Source/core/dom/TemplateContentDocumentFragment.h (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.h:42: void clearHost() { m_host = nullptr; } You can add #if !ENABLE(OILPAN) to this method.
https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... File Source/core/dom/TemplateContentDocumentFragment.h (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.h:36: static PassRefPtrWillBeRawPtr<TemplateContentDocumentFragment> create(Document& document, Element& host) On 2014/05/13 15:52:26, Mads Ager (chromium) wrote: > Why was this changed to a reference; it is used as a pointer in all of this > code. Because the pointer is guaranteed to be non 0. If the pointer is guaranteed to be non 0, Blink prefers using a reference. (Nit: I'm not a fan of the rule though.)
https://codereview.chromium.org/283663006/diff/20001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/core.gypi#ne... Source/core/core.gypi:2113: 'dom/TemplateContentDocumentFragment.h', On 2014/05/13 15:53:30, haraken wrote: > > I wonder where TemplateContentDocumentFragment.h was written before. Removed change to core.gypi. I grepped and I think it's missing. https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... File Source/core/dom/TemplateContentDocumentFragment.cpp (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.cpp:38: void TemplateContentDocumentFragment::trace(Visitor* visitor) On 2014/05/13 15:53:30, haraken wrote: > > We normally don't add a new file just for a trace() method. If we don't have a > cpp file, we can just define the trace() method in a header file. Done. https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... File Source/core/dom/TemplateContentDocumentFragment.h (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.h:36: static PassRefPtrWillBeRawPtr<TemplateContentDocumentFragment> create(Document& document, Element& host) On 2014/05/13 15:54:54, haraken wrote: > On 2014/05/13 15:52:26, Mads Ager (chromium) wrote: > > Why was this changed to a reference; it is used as a pointer in all of this > > code. > > Because the pointer is guaranteed to be non 0. If the pointer is guaranteed to > be non 0, Blink prefers using a reference. (Nit: I'm not a fan of the rule > though.) Yeah, it looked like this is always non null. https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.h:42: void clearHost() { m_host = nullptr; } On 2014/05/13 15:53:30, haraken wrote: > > You can add #if !ENABLE(OILPAN) to this method. > Done.
LGTM
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/283663006/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7392) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7158) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6648) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/7114) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/283663006/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7410) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7178) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6665) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/7132) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/283663006/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7459) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7228) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6720) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/7188) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... File Source/core/dom/TemplateContentDocumentFragment.h (right): https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... Source/core/dom/TemplateContentDocumentFragment.h:36: static PassRefPtrWillBeRawPtr<TemplateContentDocumentFragment> create(Document& document, Element& host) On 2014/05/13 16:16:21, keishi wrote: > On 2014/05/13 15:54:54, haraken wrote: > > On 2014/05/13 15:52:26, Mads Ager (chromium) wrote: > > > Why was this changed to a reference; it is used as a pointer in all of this > > > code. > > > > Because the pointer is guaranteed to be non 0. If the pointer is guaranteed to > > be non 0, Blink prefers using a reference. (Nit: I'm not a fan of the rule > > though.) > > Yeah, it looked like this is always non null. I think this would make more sense if m_host was an Element& but it is not and it will never be with oilpan. Therefore, the only thing this change does is introduce code where you need to get a reference with a '*' operator and the next thing you do it get a pointer using the '&' operator. So we are turning something into a reference and then we are storing it as a pointer. I don't think that makes sense. The only change here is that the code is harder to read and write because you have to add a '*' and a '&' and the rest of the code is unchanged and still uses a pointer.
On 2014/05/15 08:04:03, Mads Ager (chromium) wrote: > https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... > File Source/core/dom/TemplateContentDocumentFragment.h (right): > > https://codereview.chromium.org/283663006/diff/20001/Source/core/dom/Template... > Source/core/dom/TemplateContentDocumentFragment.h:36: static > PassRefPtrWillBeRawPtr<TemplateContentDocumentFragment> create(Document& > document, Element& host) > On 2014/05/13 16:16:21, keishi wrote: > > On 2014/05/13 15:54:54, haraken wrote: > > > On 2014/05/13 15:52:26, Mads Ager (chromium) wrote: > > > > Why was this changed to a reference; it is used as a pointer in all of > this > > > > code. > > > > > > Because the pointer is guaranteed to be non 0. If the pointer is guaranteed > to > > > be non 0, Blink prefers using a reference. (Nit: I'm not a fan of the rule > > > though.) > > > > Yeah, it looked like this is always non null. > > I think this would make more sense if m_host was an Element& but it is not and > it will never be with oilpan. Therefore, the only thing this change does is > introduce code where you need to get a reference with a '*' operator and the > next thing you do it get a pointer using the '&' operator. So we are turning > something into a reference and then we are storing it as a pointer. I don't > think that makes sense. The only change here is that the code is harder to read > and write because you have to add a '*' and a '&' and the rest of the code is > unchanged and still uses a pointer. Yeah, there is nothing gained. I've changed it back to a pointer.
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/283663006/60001
Message was sent while issue was closed.
Change committed as 174349 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
