Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507023002:
Support script modify iframe scrolling, marginWidth and marginHeight attr (Closed)
|
Created:
4 years, 1 month ago by chaopeng Modified:
4 years ago CC:
chromium-reviews, mlamouri+watch-blink_chromium.org, blink-reviews-html_chromium.org, dcheng, dglazkov+blink, blink-reviews, kinuko+watch, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport script modify iframe scrolling, marginWidth and marginHeight attr
For scrolling:
call FrameView::setNeddLayout in LocalFrame to update the behaviour.
For marginWidth and marginHeight:
call setIntegralAttribute to set the attr
This patch works for LocalFrame and RemoteFrame,
BUG=642603
Committed: https://crrev.com/2e671cf88d579e7a2392dff0383f47dae2655ab5
Cr-Commit-Position: refs/heads/master@{#434047}
Patch Set 1 #Patch Set 2 : add test #Patch Set 3 : add fix for marginWidth and marginHeight #
Total comments: 9
Patch Set 4 : bokan's comment addressed #Patch Set 5 : add null check #
Total comments: 4
Patch Set 6 : bokan@ comment addressed #
Total comments: 7
Patch Set 7 : bokan@ comment#19 addressed #
Total comments: 3
Patch Set 8 : alexmos@ comment#20 addressed. but layouttest still not working with flag #
Total comments: 1
Patch Set 9 : add postMessage to test #
Total comments: 1
Patch Set 10 : alexmos@ comment#22 addressed #Patch Set 11 : forgot add files #
Total comments: 1
Patch Set 12 : remove using namespace #Messages
Total messages: 38 (17 generated)
Description was changed from ========== Update iframe scrolling behaviour when script modify scrolling attr This patch is adding FrameView::setNeddLayout LocalFrame and RemoteFrame to update the behaviour. BUG=642603 ========== to ========== Support script modify iframe scrolling, marginWidth and marginHeight attr For scrolling: call FrameView::setNeddLayout in LocalFrame to update the behaviour. For marginWidth and marginHeight: call setIntegralAttribute to set the attr This patch works for LocalFrame and RemoteFrame, BUG=642603 ==========
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
bokan@chromium.org changed reviewers: + alexmos@chromium.org
+alexmos@ to double check the OOPIF changes. https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr-expected.html (right): https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr-expected.html:6: a Nit: remove 'a' https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html (right): https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html:13: <iframe id="ifr" onload="modify_attr();" scrolling="yes" I believe this will fire when the iframe is loaded, but not necessarily when the parent frame is. I would prefer if we call this from the root document's onload, that only gets called when all the children are loaded I believe. https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html:19: <script src="/resources/testharnessreport.js"></script> I don't think this test makes any use of testharness. https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (left): https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:161: // FIXME: If we are already attached, this has no effect. What did these FIXMEs mean? https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:123: contentDocument()->body()->setIntegralAttribute(marginwidthAttr, These should be done inside setMarginWidth/setMarginHeight/setScrollingMode as needed. https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebFrame.cpp (right): https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrame.cpp:154: if (frame->isLocalFrame()) { This logic is currently duplicated for Remote and Local frames. I think it'd be better if we added Document::didChangeFrameOwnerProperties(marginWidth, marginHeight, scrolling) and called it from HTMLFrameOwnerBase::setScrollingMode|MarginWidth|MarginHeight and here (before setting the RemoteFrameOwner properties so document can check for differences and issue a layout if needed, for example). https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrame.cpp:156: localFrame->document()->body()->setIntegralAttribute( We should only do each of these if the specific attribute changed.
https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (left): https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:161: // FIXME: If we are already attached, this has no effect. On 2016/11/18 20:56:18, bokan wrote: > What did these FIXMEs mean? I think it means these 3 attributes doesnot support script modify cause these 3 attributes are only used for create child frame before.
https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (left): https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:161: // FIXME: If we are already attached, this has no effect. On 2016/11/18 21:25:03, chaopeng wrote: > On 2016/11/18 20:56:18, bokan wrote: > > What did these FIXMEs mean? > > I think it means these 3 attributes doesnot support script modify cause these 3 > attributes are only used for create child frame before. Hmm, I think it was to mean that calling this function after the frame has attached to the document doesn't do anything (because they've already been parsed). But given that it's 10+ years old and doesn't have any action associated it's fine to remove. Please remove the ones left over above about name not changing.
The CQ bit was checked by chaopeng@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by chaopeng@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.
On 2016/11/18 21:37:04, bokan wrote: > https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (left): > > https://codereview.chromium.org/2507023002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:161: // FIXME: If > we are already attached, this has no effect. > On 2016/11/18 21:25:03, chaopeng wrote: > > On 2016/11/18 20:56:18, bokan wrote: > > > What did these FIXMEs mean? > > > > I think it means these 3 attributes doesnot support script modify cause these > 3 > > attributes are only used for create child frame before. > > Hmm, I think it was to mean that calling this function after the frame has > attached to the document doesn't do anything (because they've already been > parsed). But given that it's 10+ years old and doesn't have any action > associated it's fine to remove. Please remove the ones left over above about > name not changing. Updated, PTAL. Thank you.
https://codereview.chromium.org/2507023002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html (right): https://codereview.chromium.org/2507023002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html:8: var ifr = document.getElementById('ifr'); This should still be in the page's load handler as the iframe may not be loaded when this is executed. https://codereview.chromium.org/2507023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2507023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:4314: WTF::Optional<int> marginHeight, Rather than using Optionals here, always pass in the current values. You can then compare them to the frame owner's value to see if you need to set/do layout. https://codereview.chromium.org/2507023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebFrame.cpp (right): https://codereview.chromium.org/2507023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrame.cpp:152: owner->setScrollingMode(properties.scrollingMode); There's no need to check for changes when setting FrameOwner's values, this was fine as it was. https://codereview.chromium.org/2507023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrame.cpp:173: localFrame->document()->didChangeFrameOwnerProperties( If you move this up to before we set the values on RemoteFrameOwner, you can just set the values from properties and didChangeFrameOwnerProperties can check the value against the RemoteFrameOwner to see if they changed.
Updated, PTAL
Ok, this looks fine to me but I'd like someone OOPIF knowledgeable to look it over before I stamp. alexmos@, mind taking a look from here? https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:4316: DCHECK(localOwner()); The owner can be remote. You need some way to get the FrameOwner regardless if its RemoteFrameOwner or HTMLFrameOwnerElement. OOPIF reviewer might be able to help with the right way to do this. https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:827: void didChangeFrameOwnerProperties(int, int, ScrollbarMode); Sorry, didChange is a bad name since we're comparing against the old FrameOwner values. This should be willChangeFrameOwnerProperties. https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:41: #include "wtf/Optional.h" Remove. https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:121: if (contentFrame()) { Nit: no braces. https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrame.cpp (right): https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrame.cpp:25: #include "wtf/Optional.h" Remove https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrame.cpp:141: return static_cast<ScrollbarMode>(scrollingMode); just do this cast inline.
https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:4316: DCHECK(localOwner()); On 2016/11/21 16:11:36, bokan wrote: > The owner can be remote. You need some way to get the FrameOwner regardless if > its RemoteFrameOwner or HTMLFrameOwnerElement. OOPIF reviewer might be able to > help with the right way to do this. Just using frame()->owner() instead should work here. https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html (right): https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html:13: src="http://127.0.0.1:8000/fast/frames/resources/big-page.html"> Just to check: is this going to get OOPIF coverage? This is in fast/, so I *think* this will get served from a file: URL, and the iframe will still end up as an OOPIF if the test is run with --site-per-process. But can you verify? Another thing is that as of right now, only the "Site Isolation Win" FYI bot runs all layout tests with --site-per-process. Our other bots (including the linux_site_isolation trybot) only run the http/tests subset of layout tests. So ideally, to get coverage on those bots also, we'd switch this test to localhost:8000 and move the test over to http/tests. Is that possible/easy to do? https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:827: void willChangeFrameOwnerProperties(int, int, ScrollbarMode); nit: might be better to include the arg names here for the two ints, as it's not obvious what they refer to. https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrame.cpp (right): https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrame.cpp:150: if (localFrame->document()) { I don't think you need this null check.
On 2016/11/21 17:56:56, alexmos wrote: > https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/2507023002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Document.cpp:4316: DCHECK(localOwner()); > On 2016/11/21 16:11:36, bokan wrote: > > The owner can be remote. You need some way to get the FrameOwner regardless if > > its RemoteFrameOwner or HTMLFrameOwnerElement. OOPIF reviewer might be able to > > help with the right way to do this. > > Just using frame()->owner() instead should work here. > > https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html > (right): > > https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/frames/script-modify-iframe-attr.html:13: > src="http://127.0.0.1:8000/fast/frames/resources/big-page.html"> > Just to check: is this going to get OOPIF coverage? This is in fast/, so I > *think* this will get served from a file: URL, and the iframe will still end up > as an OOPIF if the test is run with --site-per-process. But can you verify? > > Another thing is that as of right now, only the "Site Isolation Win" FYI bot > runs all layout tests with --site-per-process. Our other bots (including the > linux_site_isolation trybot) only run the http/tests subset of layout tests. So > ideally, to get coverage on those bots also, we'd switch this test to > localhost:8000 and move the test over to http/tests. Is that possible/easy to > do? > > https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Document.h (right): > > https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Document.h:827: void > willChangeFrameOwnerProperties(int, int, ScrollbarMode); > nit: might be better to include the arg names here for the two ints, as it's not > obvious what they refer to. > > https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/WebFrame.cpp (right): > > https://codereview.chromium.org/2507023002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/WebFrame.cpp:150: if (localFrame->document()) { > I don't think you need this null check. Updated, PTAL. Thank you.
https://codereview.chromium.org/2507023002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html (right): https://codereview.chromium.org/2507023002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html:1: <!DOCTYPE html> Perhaps this should go under http/tests/misc instead? It doesn't look like http/tests/frames exists today. https://codereview.chromium.org/2507023002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html (right): https://codereview.chromium.org/2507023002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html:17: ifr.contentWindow.postMessage({}, '*'); I think the scrolling/margin attributes should be changed before sending this postMessage. I.e., the goal of the postMessage would be to guarantee that the OOPIF has seen and processed scrolling/margin updates (since the postMessage IPC would be processed after FrameMsg_SetFrameOwnerProperties). I think that should help to make this pass with --site-per-process once we fix https://bugs.chromium.org/p/chromium/issues/detail?id=667551.
On 2016/11/22 05:13:11, alexmos wrote: > https://codereview.chromium.org/2507023002/diff/140001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html > (right): > > https://codereview.chromium.org/2507023002/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html:1: > <!DOCTYPE html> > Perhaps this should go under http/tests/misc instead? It doesn't look like > http/tests/frames exists today. > > https://codereview.chromium.org/2507023002/diff/160001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html > (right): > > https://codereview.chromium.org/2507023002/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html:17: > ifr.contentWindow.postMessage({}, '*'); > I think the scrolling/margin attributes should be changed before sending this > postMessage. I.e., the goal of the postMessage would be to guarantee that the > OOPIF has seen and processed scrolling/margin updates (since the postMessage IPC > would be processed after FrameMsg_SetFrameOwnerProperties). I think that should > help to make this pass with --site-per-process once we fix > https://bugs.chromium.org/p/chromium/issues/detail?id=667551. Updated, PTAL.
On 2016/11/22 15:10:02, chaopeng wrote: > On 2016/11/22 05:13:11, alexmos wrote: > > > https://codereview.chromium.org/2507023002/diff/140001/third_party/WebKit/Lay... > > File > > > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html > > (right): > > > > > https://codereview.chromium.org/2507023002/diff/140001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html:1: > > <!DOCTYPE html> > > Perhaps this should go under http/tests/misc instead? It doesn't look like > > http/tests/frames exists today. > > > > > https://codereview.chromium.org/2507023002/diff/160001/third_party/WebKit/Lay... > > File > > > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html > > (right): > > > > > https://codereview.chromium.org/2507023002/diff/160001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html:17: > > ifr.contentWindow.postMessage({}, '*'); > > I think the scrolling/margin attributes should be changed before sending this > > postMessage. I.e., the goal of the postMessage would be to guarantee that the > > OOPIF has seen and processed scrolling/margin updates (since the postMessage > IPC > > would be processed after FrameMsg_SetFrameOwnerProperties). I think that > should > > help to make this pass with --site-per-process once we fix > > https://bugs.chromium.org/p/chromium/issues/detail?id=667551. > > Updated, PTAL. It seems the new layout test has disappeared from the latest patchset. Did you forget to add it?
On 2016/11/22 17:21:03, alexmos wrote: > On 2016/11/22 15:10:02, chaopeng wrote: > > On 2016/11/22 05:13:11, alexmos wrote: > > > > > > https://codereview.chromium.org/2507023002/diff/140001/third_party/WebKit/Lay... > > > File > > > > > > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html > > > (right): > > > > > > > > > https://codereview.chromium.org/2507023002/diff/140001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html:1: > > > <!DOCTYPE html> > > > Perhaps this should go under http/tests/misc instead? It doesn't look like > > > http/tests/frames exists today. > > > > > > > > > https://codereview.chromium.org/2507023002/diff/160001/third_party/WebKit/Lay... > > > File > > > > > > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html > > > (right): > > > > > > > > > https://codereview.chromium.org/2507023002/diff/160001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/http/tests/frames/script-modify-iframe-attr.html:17: > > > ifr.contentWindow.postMessage({}, '*'); > > > I think the scrolling/margin attributes should be changed before sending > this > > > postMessage. I.e., the goal of the postMessage would be to guarantee that > the > > > OOPIF has seen and processed scrolling/margin updates (since the postMessage > > IPC > > > would be processed after FrameMsg_SetFrameOwnerProperties). I think that > > should > > > help to make this pass with --site-per-process once we fix > > > https://bugs.chromium.org/p/chromium/issues/detail?id=667551. > > > > Updated, PTAL. > > It seems the new layout test has disappeared from the latest patchset. Did you > forget to add it? Sorry, I forgot to add that files. Updated, PTAL. Thank you.
LGTM
https://codereview.chromium.org/2507023002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrame.cpp (right): https://codereview.chromium.org/2507023002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrame.cpp:29: using namespace HTMLNames; Is this necessary, along with the header above?
On 2016/11/22 19:43:35, alexmos wrote: > https://codereview.chromium.org/2507023002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/WebFrame.cpp (right): > > https://codereview.chromium.org/2507023002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/WebFrame.cpp:29: using namespace HTMLNames; > Is this necessary, along with the header above? No, I should remove this.
LGTM
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2507023002/#ps220001 (title: "remove using namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1479855965924370,
"parent_rev": "d02a345524675311c68066ff51776a16d8bc4640", "commit_rev":
"2a26d56b5c80c9b673057a199c1b3f3f114860a0"}
Message was sent while issue was closed.
Description was changed from ========== Support script modify iframe scrolling, marginWidth and marginHeight attr For scrolling: call FrameView::setNeddLayout in LocalFrame to update the behaviour. For marginWidth and marginHeight: call setIntegralAttribute to set the attr This patch works for LocalFrame and RemoteFrame, BUG=642603 ========== to ========== Support script modify iframe scrolling, marginWidth and marginHeight attr For scrolling: call FrameView::setNeddLayout in LocalFrame to update the behaviour. For marginWidth and marginHeight: call setIntegralAttribute to set the attr This patch works for LocalFrame and RemoteFrame, BUG=642603 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Support script modify iframe scrolling, marginWidth and marginHeight attr For scrolling: call FrameView::setNeddLayout in LocalFrame to update the behaviour. For marginWidth and marginHeight: call setIntegralAttribute to set the attr This patch works for LocalFrame and RemoteFrame, BUG=642603 ========== to ========== Support script modify iframe scrolling, marginWidth and marginHeight attr For scrolling: call FrameView::setNeddLayout in LocalFrame to update the behaviour. For marginWidth and marginHeight: call setIntegralAttribute to set the attr This patch works for LocalFrame and RemoteFrame, BUG=642603 Committed: https://crrev.com/2e671cf88d579e7a2392dff0383f47dae2655ab5 Cr-Commit-Position: refs/heads/master@{#434047} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2e671cf88d579e7a2392dff0383f47dae2655ab5 Cr-Commit-Position: refs/heads/master@{#434047}
Message was sent while issue was closed.
Description was changed from ========== Support script modify iframe scrolling, marginWidth and marginHeight attr For scrolling: call FrameView::setNeddLayout in LocalFrame to update the behaviour. For marginWidth and marginHeight: call setIntegralAttribute to set the attr This patch works for LocalFrame and RemoteFrame, BUG=642603 Committed: https://crrev.com/2e671cf88d579e7a2392dff0383f47dae2655ab5 Cr-Commit-Position: refs/heads/master@{#434047} ========== to ========== Support script modify iframe scrolling, marginWidth and marginHeight attr For scrolling: call FrameView::setNeddLayout in LocalFrame to update the behaviour. For marginWidth and marginHeight: call setIntegralAttribute to set the attr This patch works for LocalFrame and RemoteFrame, BUG=642603 Committed: https://crrev.com/2e671cf88d579e7a2392dff0383f47dae2655ab5 Cr-Commit-Position: refs/heads/master@{#434047} ========== |
