Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2549443003:
Move <marquee> implementation to HTMLMarqueeElement.cpp (Closed)
DescriptionMove <marquee> implementation to HTMLMarqueeElement.cpp
- Moves HTMLMarqueeElement off Blink-in-JS
- Reimplements HTMLMarqueeElement in C++: implementation matches
HTMLMarqueeElement.js and uses standard DOM API wherever possible
- Removes HTMLMarqueeElement.js
BUG=669656, 668618
Committed: https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1
Cr-Commit-Position: refs/heads/master@{#436628}
Patch Set 1 #Patch Set 2 : Move out transform string creation into separate method #
Total comments: 29
Patch Set 3 : Address feedback #
Total comments: 15
Patch Set 4 : Code review #Patch Set 5 : Add layout tests #
Total comments: 14
Patch Set 6 : Fix nits #
Total comments: 4
Patch Set 7 : Initialize attributes in insertedInto #
Total comments: 2
Patch Set 8 : Add asserts for didParse #
Total comments: 22
Messages
Total messages: 58 (30 generated)
The CQ bit was checked by adithyas@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...
Description was changed from ========== Move <marquee> implementation to HTMLMarqueeElement.cpp - Moves HTMLMarqueeElement off Blink-on-JS - Reimplements HTMLMarqueeElement in C++: implementation matches HTMLMarqueeElement.js and uses standard DOM API wherever possible - Removes HTMLMarqueeElement.js BUG=669656 ========== to ========== Move <marquee> implementation to HTMLMarqueeElement.cpp - Moves HTMLMarqueeElement off Blink-in-JS - Reimplements HTMLMarqueeElement in C++: implementation matches HTMLMarqueeElement.js and uses standard DOM API wherever possible - Removes HTMLMarqueeElement.js BUG=669656 ==========
adithyas@chromium.org changed reviewers: + haraken@chromium.org, jbroman@chromium.org
The CQ bit was checked by adithyas@chromium.org
I've added in some inline comments to explain some layout test related changes. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/inline/inline-marquee-crash-expected.txt (left): https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/inline/inline-marquee-crash-expected.txt:1: CONSOLE WARNING: line 361: Invalid keyframe value for property transform: translateX(NaNpx) The console warning emitted was a result of NaN passed as a transform parameter [getComputedStyle(marquee) returns "auto" for width and height which is parsed as NaN by parseInt]. In the C++ implementation, the animation is not started if the width and height are "auto", so the warning isn't emitted. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:72: ":host > div { will-change: transform; }"); "will-change: transform" is added in to force an additional layer for the div to be created. The layers created in fast/block/float/marquee-shrink-to-avoid-float.html will not match with the expected result without this (the div gets absorbed into the marquee's layer).
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by adithyas@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.
(sheriffing; sorry for the slow reviews today and tomorrow) Some thoughts. Still need to have a closer look at the metrics calculation logic. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:65: createShadowRootInternal(ShadowRootType::V0, ASSERT_NO_EXCEPTION); Ultimately this should probably be a user agent shadow root, in which case the usual pattern is to call ensureUserAgentShadowRoot in/after the constructor, and then do the setup of the shadow root in an override of didAddUserAgentShadowRoot. To me this is essentially equivalent to the public APIs (except that it creates a UA root, which we probably should have anyhow -- there's a bug to that effect). I'd be OK with this in a followup. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:72: ":host > div { will-change: transform; }"); On 2016/12/01 at 20:30:48, adithyas wrote: > "will-change: transform" is added in to force an additional layer for the div to be created. The layers created in fast/block/float/marquee-shrink-to-avoid-float.html will not match with the expected result without this (the div gets absorbed into the marquee's layer). To elaborate slightly, this layer gets created anyhow when the animation begins. We believe the issue is a timing issue with when that style actually applies. It's simpler to have the creation of a paint layer be stable, anyhow. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:99: std::vector<QualifiedName> presentationalAttributes = { as a rule we prefer WTF::Vector to std::vector inside Blink. In this particular case, an array of const pointers will do: static const QualifiedName* presentationalAttributes[] = { &HTMLNames::bgcolorAttr, &HTMLNames::heightAttr, ... }; for (const auto* attr : presentationalAttributes) { initializeAttribute(*attr); } https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:106: this->start(); nit: in C++, prefer "start();" to "this->start();" (and similar cases elsewhere, except where it's not legal due to variable name collision) https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:126: if (!ok || scrollAmount < 0) { not-really-a-nit: Here and elsewhere, you're free to omit the braces around one-line if statements. Up to you. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:135: exceptionState.throwDOMException( Here and elsewhere: ExceptionState::throwDOMException prepares to throw an exception, but it doesn't actually stop execution of the current one (we don't have real exceptions in C++). You should return after each of these, if you don't want the rest of the function to execute. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:185: if (document().hasSVGRootNode()) I wonder if this still applies. Keeping it for now would be reasonable (besides, I suspect html:marquee tags in svg are essentially nonexistent), but this no longer runs script, so if that's the constraint this might not be necessary anymore. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:218: style()->setProperty("bgcolor", newValue, String(), ASSERT_NO_EXCEPTION); The CSS property is background-color, not bgcolor. Also, below, I thought setProperty took hyphenated-names rather than camelCase ones like we expose to script (V8CSSStyleDeclarationCustom.cpp does the conversion for JS), so "margin-left", "margin-right", etc. below. nit (but wait for someone else to weight in): This is closer to the web API, but we would ordinarily use Element::setInlineStyleProperty in C++ (which takes a CSSPropertyID enum, and and so makes this particular bug a compile error). https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:259: keyframes.append(keyframe1); nit: when passing ownership of a RefPtr, prefer to move it to avoid having to increment and decrement the ref count: keyframes.append(std::move(keyframe1)); super-nit: Or if you want to be slightly more fancy, you could even just pass {std::move(keyframe1), std::move(keyframe2)} in the place of having a |keyframes| variable. aside: We could use a more type-safe type of keyframe here (using AnimatableTransform), but I have no strong feelings one way or another (this way is closer to the web's view, so this seems fine). https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:268: return effectModel; super-nit: might as well just return StringKeyframeEffectModel::create(keyframes, LinearTimingFunction::shared()); https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:272: void HTMLMarqueeElement::initializeTiming(Timing& timing, double duration) { You can just return Timing from this method. Or better yet, just inline it (especially if the below things just make it small). https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:273: TimingInput::setFillMode(timing, "forwards"); I'd just use the enum etc here instead of converting from known strings. We're pretty close to the exact web APIs already here; I think the difference is just a matter of JavaScript APIs liking strings and C++ APIs liking enums. timing.fillMode = Timing::FillMode::FORWARDS; timing.iterationDuration = duration; (assuming you've already ruled out negative numbers and NaN) https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:306: player->setId(""); nit: "player->setId(emptyString);" is slightly more explicit and slightly more efficient
I haven't looked at the details, but the overall approach looks good. Just check that this website works with your CL: http://www.fillster.com/htmlcodes/marquees.html The coverage of <marquee> in layout tests is too low.
The CQ bit was checked by adithyas@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...
haraken@: I tested with that link and the behavior seems to match. Also did some manual tests with the hspace/vspace attributes, which aren't tested by that link. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:65: createShadowRootInternal(ShadowRootType::V0, ASSERT_NO_EXCEPTION); Yup, I'd prefer to do this is in a follow-up, especially if it's fixing a bug. I did however want to make sure that calling createShadowRootInternal matches current behaviour. I could not call createShadowRoot() directly as it requires a ScriptState object which I don't have access to. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:99: std::vector<QualifiedName> presentationalAttributes = { Fixed. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:106: this->start(); Fixed, here and everywhere else. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:126: if (!ok || scrollAmount < 0) { Removed the braces everywhere I didn't need them. Sorry, I'm just very inconsistent with these :P https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:135: exceptionState.throwDOMException( On 2016/12/01 at 22:22:52, jbroman wrote: > Here and elsewhere: ExceptionState::throwDOMException prepares to throw an exception, but it doesn't actually stop execution of the current one (we don't have real exceptions in C++). You should return after each of these, if you don't want the rest of the function to execute. Forgot about that! Fixed. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:185: if (document().hasSVGRootNode()) Removed it, it's not necessary anymore. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:218: style()->setProperty("bgcolor", newValue, String(), ASSERT_NO_EXCEPTION); You are correct, they do use the hyphenated names, the hspace/vspace attributes did not work with the camel case versions. I think based on this, I'd much prefer using Element::setInlineStyleProperty, but I'll wait. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:259: keyframes.append(keyframe1); Fixed! I'm going to keep using StringKeyFrame unless anyone has any strong objections. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:268: return effectModel; Fixed. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:272: void HTMLMarqueeElement::initializeTiming(Timing& timing, double duration) { removed the method https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:273: TimingInput::setFillMode(timing, "forwards"); I've changed it to use the enum directly for fill mode. I'm still going to use TimingInput::setIterationDuration though, because iterationDuration needs to be microseconds and duration is in milliseconds (and this wasn't obvious to me). setIterationDuration does the conversion. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:306: player->setId(""); Fixed (assuming you meant emptyString())
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm This seems like a faithful conversion of the current contents of HTMLMarqueeElement.js. The stringyness of the web-exposed APIs feels a little strange in a C++ context, but if people are motivated to change that later, it's easy to do so. Similarly, we'd usually do presentation attributes using isPresentationAttribute and collectStyleForPresentationAttribute -- which is technically more correct for some edge-casey reasons like legacy color parsing and whether the style is reflected back to the style property, but it's probably better to not change behavior at the moment. Agreed that it would be good to have more layout tests for <marquee>. It seems like we're missing some of the basics, even, like whether the bgcolor attribute works at all. Some simple ref tests for the presentational attributes wouldn't hurt (i.e. comparing a stopped <marquee bgcolor=green>hi</marquee> to <div style="background-color: green">hi</div>). You'll need a signoff for third_party/WebKit/public/ and content/child/ still. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:273: TimingInput::setFillMode(timing, "forwards"); On 2016/12/02 at 19:29:49, adithyas wrote: > I've changed it to use the enum directly for fill mode. I'm still going to use TimingInput::setIterationDuration though, because iterationDuration needs to be microseconds and duration is in milliseconds (and this wasn't obvious to me). setIterationDuration does the conversion. Ah, OK. https://codereview.chromium.org/2549443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:306: player->setId(""); On 2016/12/02 at 19:29:49, adithyas wrote: > Fixed (assuming you meant emptyString()) Indeed. https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:314: const AtomicString& bhvr = getAttribute(HTMLNames::behaviorAttr); "bhvr" and "lp" (above) are a bit too terse for Blink style. "behaviorString" and "totalLoopCount" or something like that? https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:424: AtomicString transform = parameters.transformBegin; nit: more concisely (and IMHO, clearly): std::swap(parameters.transformBegin, parameters.transformEnd); https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:495: AtomicString HTMLMarqueeElement::createTransform(bool isNegative, Why the |isNegative| parameter, instead of simple passing the negation of |value|? This would also mean you wouldn't need the if statement here. https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:499: return AtomicString(String::format("translate%c(-%fpx)", axis, value)); If you change the other thing to String, you can remove this conversion to AtomicString (and change the function signature). https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.h (right): https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:106: AtomicString transformBegin; These should probably be String rather than AtomicString (it's what StringKeyframe::setCSSPropertyValue takes, anyhow).
LGTM Maybe do we want to send an Intent-to-remove for Blink-in-JS? (I can do that.) https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:323: const AtomicString& dir = getAttribute(HTMLNames::directionAttr); direction https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.h (right): https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:70: RequestAnimationFrameCallback(HTMLMarqueeElement* marquee) Add explicit. https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:87: AnimationFinished(HTMLMarqueeElement* marquee) Add explicit.
https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:314: const AtomicString& bhvr = getAttribute(HTMLNames::behaviorAttr); On 2016/12/03 at 23:47:31, jbroman wrote: > "bhvr" and "lp" (above) are a bit too terse for Blink style. "behaviorString" and "totalLoopCount" or something like that? Fixed. https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:323: const AtomicString& dir = getAttribute(HTMLNames::directionAttr); On 2016/12/05 at 02:10:53, haraken wrote: > direction Fixed. https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:424: AtomicString transform = parameters.transformBegin; On 2016/12/03 at 23:47:31, jbroman wrote: > nit: more concisely (and IMHO, clearly): > > std::swap(parameters.transformBegin, parameters.transformEnd); Fixed, thanks. https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:495: AtomicString HTMLMarqueeElement::createTransform(bool isNegative, On 2016/12/03 at 23:47:31, jbroman wrote: > Why the |isNegative| parameter, instead of simple passing the negation of |value|? This would also mean you wouldn't need the if statement here. I have no idea why I did something so convoluted, fixed :) https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:499: return AtomicString(String::format("translate%c(-%fpx)", axis, value)); On 2016/12/03 at 23:47:31, jbroman wrote: > If you change the other thing to String, you can remove this conversion to AtomicString (and change the function signature). Fixed. https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.h (right): https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:70: RequestAnimationFrameCallback(HTMLMarqueeElement* marquee) On 2016/12/05 at 02:10:53, haraken wrote: > Add explicit. Added, here and below. https://codereview.chromium.org/2549443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:106: AtomicString transformBegin; On 2016/12/03 at 23:47:31, jbroman wrote: > These should probably be String rather than AtomicString (it's what StringKeyframe::setCSSPropertyValue takes, anyhow). Fixed.
adithyas@chromium.org changed reviewers: + esprehn@chromium.org
+esprehn@ for content/child review
jbroman@: I added a couple of layout tests, could you PTAL?
The CQ bit was checked by adithyas@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...
You need to use parseAttribute instead of attributeChangedCallback, you don't need to parse the attributes inside the connected callback anymore as well. https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:67: ShadowRoot* shadow = ensureUserAgentShadowRoot(), see HTMLSummaryElement::create https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:82: m_mover = mover; This all goes inside didAddUserAgentShadowRoot https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:126: int HTMLMarqueeElement::scrollAmount() { const https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:145: int HTMLMarqueeElement::scrollDelay() { const https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:164: int HTMLMarqueeElement::loop() { const https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:202: void HTMLMarqueeElement::initializeAttribute(const QualifiedName& attr) { remove this https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:209: void HTMLMarqueeElement::attributeChangedCallback(const QualifiedName& attr, You need to use ::parseAttribute now, previously the custom element system was calling attributeChangedCallback, but it's not doing that now. This patch doesn't handle dynamic changes properly. https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.h (right): https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:66: class RequestAnimationFrameCallback : public FrameRequestCallback { final https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:83: class AnimationFinished : public EventListener { final
https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:94: attributeChangedCallback(name, newValue); I see, you do actually call attributeChangedCallback for dynamic changes. Okay no need to switch to parseAttribute in this patch, please fix the other nits and then this patch looks good.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:67: ShadowRoot* shadow = On 2016/12/05 at 19:19:42, esprehn wrote: > ensureUserAgentShadowRoot(), see HTMLSummaryElement::create jbroman@ pointed this out as well; I want to change this in a follow-up CL since it's a behavior change and I believe it fixes a current bug (http://crbug.com/420650). https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:126: int HTMLMarqueeElement::scrollAmount() { On 2016/12/05 at 19:19:42, esprehn wrote: > const Done, here and for the other 2 methods. https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:202: void HTMLMarqueeElement::initializeAttribute(const QualifiedName& attr) { On 2016/12/05 at 19:19:42, esprehn wrote: > remove this I removed the call to this function from ::insertedInto, but it looks like I still need to call this inside attributeChangedCallback to handle a change to the marquee element's "style" attribute. https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.h (right): https://codereview.chromium.org/2549443003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:66: class RequestAnimationFrameCallback : public FrameRequestCallback { On 2016/12/05 at 19:19:42, esprehn wrote: > final Added, here and below.
layout test lgtm; I'm happy when esprehn is :)
https://codereview.chromium.org/2549443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:195: void HTMLMarqueeElement::initializeAttribute(const QualifiedName& attr) { Lets just inline this if you're only going to have one caller of it. https://codereview.chromium.org/2549443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:224: } else if (attr == HTMLNames::styleAttr) { You're saying when I do marquee.style.foo = "..." you're going to go read through every attribute of (bgcolor, height, hspace, vspace, width) and reassign the style property of the marquee itself? That doesn't appear to be what the old code did. m = document.createElement("marquee"); document.body.appendChild(m); m.height = 10; // m.style.height == "10px" m.style.height = "20px"; // m.style.height == "20px"; // m.getAttribute("style") == "height: 20px" Note that in general this entire function should be replaced with presentationAttributeStyle (in another patch) which hides the inline style from the author.
https://codereview.chromium.org/2549443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:224: } else if (attr == HTMLNames::styleAttr) { On 2016/12/05 at 22:01:37, esprehn wrote: > You're saying when I do marquee.style.foo = "..." you're going to go read through every attribute of (bgcolor, height, hspace, vspace, width) and reassign the style property of the marquee itself? That doesn't appear to be what the old code did. The old code took advantage of the fact that insertedNode was called after attributeChanged and initialized bgcolor, height, hspace, vspace and width for a second time). So for <marquee bgcolor="green" style="padding: 10px;">foo</marquee>: -> attributeChanged would be called for bgcolor, and background-color="green" would be inlined. -> attributeChanged would be called for style, and background-color="green" would be removed completely and replaced with "padding: 10px" -> insertedNode would be called and background-color="green" would be readded to the inline style I initially had replicated the old functionality, but I thought this might be more reasonable. I can go back to the old functionality if you think that's better. I'm assuming though that if you set the bgcolor attribute to a color, changing the inline style of the marquee should not affect the background color, which would happen with the old implementation. > Note that in general this entire function should be replaced with presentationAttributeStyle (in another patch) which hides the inline style from the author. Perhaps using presentationAttribute style will simplify things a lot :)
I see, lets go back to what the old code did and call the initialize function inside insertedInto. Just replicate what it did exactly without using the internal C++ DOM hooks. Then future patches can clean this up. This current patch is introducing new different behavior that also diverges from the direction we want to go, so lets back up a step (sorry for the churn).
I've changed it back to initializing attributes inside insertedInto. https://codereview.chromium.org/2549443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:195: void HTMLMarqueeElement::initializeAttribute(const QualifiedName& attr) { On 2016/12/05 at 22:01:37, esprehn wrote: > Lets just inline this if you're only going to have one caller of it. Done.
The CQ bit was checked by adithyas@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alancutter@chromium.org changed reviewers: + alancutter@chromium.org
Animation API lgtm with comment. Please add 668618 to the list of bugs fixed by this patch. https://codereview.chromium.org/2549443003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:254: styleSheetContents); Add asserts for didParse on the return value of setCSSPropertyValue().
Description was changed from ========== Move <marquee> implementation to HTMLMarqueeElement.cpp - Moves HTMLMarqueeElement off Blink-in-JS - Reimplements HTMLMarqueeElement in C++: implementation matches HTMLMarqueeElement.js and uses standard DOM API wherever possible - Removes HTMLMarqueeElement.js BUG=669656 ========== to ========== Move <marquee> implementation to HTMLMarqueeElement.cpp - Moves HTMLMarqueeElement off Blink-in-JS - Reimplements HTMLMarqueeElement in C++: implementation matches HTMLMarqueeElement.js and uses standard DOM API wherever possible - Removes HTMLMarqueeElement.js BUG=669656,668618 ==========
https://codereview.chromium.org/2549443003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:254: styleSheetContents); On 2016/12/06 at 04:26:57, alancutter wrote: > Add asserts for didParse on the return value of setCSSPropertyValue(). Done.
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jbroman@chromium.org, esprehn@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2549443003/#ps140001 (title: "Add asserts for didParse")
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": 140001, "attempt_start_ts": 1481039024913920,
"parent_rev": "4ab713ec865f408b1c4df2a35acd254d1d7587cd", "commit_rev":
"23342f799594962314b1add3089fa13e0283e3f7"}
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Move <marquee> implementation to HTMLMarqueeElement.cpp - Moves HTMLMarqueeElement off Blink-in-JS - Reimplements HTMLMarqueeElement in C++: implementation matches HTMLMarqueeElement.js and uses standard DOM API wherever possible - Removes HTMLMarqueeElement.js BUG=669656,668618 ========== to ========== Move <marquee> implementation to HTMLMarqueeElement.cpp - Moves HTMLMarqueeElement off Blink-in-JS - Reimplements HTMLMarqueeElement in C++: implementation matches HTMLMarqueeElement.js and uses standard DOM API wherever possible - Removes HTMLMarqueeElement.js BUG=669656,668618 Committed: https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1 Cr-Commit-Position: refs/heads/master@{#436628} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1 Cr-Commit-Position: refs/heads/master@{#436628}
Message was sent while issue was closed.
tkent@chromium.org changed reviewers: + tkent@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:90: void HTMLMarqueeElement::attributeChanged(const QualifiedName& name, Overriding attributeChanged looks weird. Doesn't parseAttribute() work? https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:107: const AtomicString& value = getAttribute(*attr); getAttribute -> fastGetAttribute https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:133: int scrollAmount = getAttribute(HTMLNames::scrollamountAttr).toInt(&ok); Please do not use AtomicString::toInt(). It isn't compatible with the HTML standard, and has an integer-overflow bug. We should use parseHTMLInteger(). Probably we should introduce Element::getUnsignedIntegralAttribute(const QualifiedName&, int fallbackValue). https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:245: AnimationParameters& parameters) { The argument type should be |const AnimationParameters&|. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:316: if (behavior == "alternate") Should it be case-sensitive match? https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:335: return hasAttribute(HTMLNames::truespeedAttr); hasAttribute -> fastHasAttribute https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:483: return AtomicString(String::format("translate%c(%fpx)", axis, value)); Does this work in French locale, which use ',' for decimal point? https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.h (right): https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:66: class RequestAnimationFrameCallback final : public FrameRequestCallback { This class definition should be moved to HTMLMarqueeElement.cpp. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:83: class AnimationFinished final : public EventListener { This class definition should be moved to HTMLMarqueeElement.cpp. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:126: enum Behavior { Scroll, Slide, Alternate }; Please prepend 'k' to enum members. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:127: Behavior behavior() const; This will prevent naming rule conversion coming soon because |behavior()| will be renamed to |Behavior()|. This should be |getBehavior()|. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:129: enum Direction { Left, Right, Up, Down }; Please prepend 'k' to enum members. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:130: Direction direction() const; This will prevent naming rule conversion coming soon because |direction()| will be renamed to |Direction()|. This should be |getDirection()|. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:132: bool trueSpeed() const; I don't think this function is necessary. It's used only once.
Message was sent while issue was closed.
Please fix toInt() to not have an overflow instead of creating some wrapper around it. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:90: void HTMLMarqueeElement::attributeChanged(const QualifiedName& name, On 2016/12/06 at 22:43:11, tkent wrote: > Overriding attributeChanged looks weird. Doesn't parseAttribute() work? Yes see the discussion for follow up patches https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:107: const AtomicString& value = getAttribute(*attr); On 2016/12/06 at 22:43:11, tkent wrote: > getAttribute -> fastGetAttribute We were trying to avoid using private hooks that authors wouldn't have access to, but yeah the rest of the DOM C++ uses this. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:133: int scrollAmount = getAttribute(HTMLNames::scrollamountAttr).toInt(&ok); On 2016/12/06 at 22:43:11, tkent wrote: > Please do not use AtomicString::toInt(). It isn't compatible with the HTML standard, and has an integer-overflow bug. We should use parseHTMLInteger(). > > Probably we should introduce Element::getUnsignedIntegralAttribute(const QualifiedName&, int fallbackValue). Please fix the toInt() to not have an overflow. :) https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.h (right): https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:132: bool trueSpeed() const; On 2016/12/06 at 22:43:11, tkent wrote: > I don't think this function is necessary. It's used only once. This is web exposed API.
Message was sent while issue was closed.
I've addressed most of tkent's comments in this CL: http://crrev.com/2554403002 https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp (right): https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:133: int scrollAmount = getAttribute(HTMLNames::scrollamountAttr).toInt(&ok); On 2016/12/07 at 00:10:00, esprehn wrote: > On 2016/12/06 at 22:43:11, tkent wrote: > > Please do not use AtomicString::toInt(). It isn't compatible with the HTML standard, and has an integer-overflow bug. We should use parseHTMLInteger(). > > > > Probably we should introduce Element::getUnsignedIntegralAttribute(const QualifiedName&, int fallbackValue). > > Please fix the toInt() to not have an overflow. :) I'm not really sure about the overflow bug, I couldn't reproduce it (assuming this is the bug you're talking about: http://crbug.com/666681). Also parseHTMLInteger doesn't work in this scenario because there's no way of knowing if the actual user input was NaN (in which case, the default value should be returned instead of 0). https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:316: if (behavior == "alternate") On 2016/12/06 at 22:43:11, tkent wrote: > Should it be case-sensitive match? Nope, this is a bug, thanks for catching it. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp:483: return AtomicString(String::format("translate%c(%fpx)", axis, value)); On 2016/12/06 at 22:43:11, tkent wrote: > Does this work in French locale, which use ',' for decimal point? I don't think it does, thanks for pointing this out. https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMarqueeElement.h (right): https://codereview.chromium.org/2549443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMarqueeElement.h:132: bool trueSpeed() const; On 2016/12/07 at 00:10:00, esprehn wrote: > On 2016/12/06 at 22:43:11, tkent wrote: > > I don't think this function is necessary. It's used only once. > > This is web exposed API. This function is private, the web exposed API is implemented with [Reflect], so I'll just inline this. |
