New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 669656 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 420650
issue 424613
issue 455596
issue 666520



Sign in to add a comment

Move HTMLMarqueeElement off of private script

Project Member Reported by jbroman@chromium.org, Nov 29 2016

Issue description

Per esprehn@, Blink-in-JS is on the way out and has only two remaining uses (HTMLMarqueeElement and the XML tree viewer). This bug tracks the effort to move HTMLMarqueeElement.js into C++. (It can continue using web animations.)

adithyas@ volunteered to work on this.
 

Comment 1 by tkent@chromium.org, Nov 29 2016

Blocking: 666520 420650 424613

Comment 2 by tkent@chromium.org, Nov 29 2016

I recommend to revert to pre-Blink-in-JS C++ implemnentation, rather than translating the current Blink-in-JS implementation to C++. Then, adopting web animations.

Blink-in-JS marquee caused a lot of regressions. If we translate HTMLMarqueeElement.js to C++, we need to pay cost to fix the regressions.
https://bugs.chromium.org/p/chromium/issues/list?q=component:Blink%3EHTML%3EMarquee

I'm not sure we want to resurrect the complexity that LayoutMarquee had, especially 17 major releases (two years) after it was released. And so it hasn't been updated for a number of refactors in the meantime, like Slimming Paint.

About half of those bugs seem to result fairly clearly from the use of a private script context, and at least one seems obsolete. The rest don't seem to have been prioritized in the last two years.

Since <marquee> is evidently an unloved feature, it seems better to me to have it piggyback on well-supported code (web animations) than reintroducing the custom layout and timing code that we had before.
I think our plan is to replace Blink-in-JS with web-modules, right? Dimitri is now implementing the infrastructure of web-modules.

In any case, it's a good idea to rewrite <marquee> and XMLViewer in C++ on top of existing Web-exposed APIs (but in C++). Once web-modules is available, we can migrate the C++ implementation to web-modules easily.

Cc: dglazkov@chromium.org
+1 on rewriting it in C++ using standard DOM APIs. I was going to try that over Christmas vacation, but please don't let me hold you back :)

As I am looking at Web Modules (nee Web Agents), I think they aren't looking like a good fit anymore -- they are our way to let embedders (Chrome) introspect and interact with the page.
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a95f76a0318a32bf5bf1153fb291402806c298d1

commit a95f76a0318a32bf5bf1153fb291402806c298d1
Author: adithyas <adithyas@chromium.org>
Date: Tue Dec 06 17:41:43 2016

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 

Review-Url: https://codereview.chromium.org/2549443003
Cr-Commit-Position: refs/heads/master@{#436628}

[modify] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/content/child/blink_platform_impl.cc
[add] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/LayoutTests/fast/html/marquee-bgcolor-expected.html
[add] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/LayoutTests/fast/html/marquee-bgcolor.html
[add] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/LayoutTests/fast/html/marquee-vspace-hspace-expected.html
[add] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/LayoutTests/fast/html/marquee-vspace-hspace.html
[modify] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/LayoutTests/fast/inline/inline-marquee-crash-expected.txt
[modify] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp
[modify] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/Source/core/html/HTMLMarqueeElement.h
[modify] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/Source/core/html/HTMLMarqueeElement.idl
[delete] https://crrev.com/63ab6f6ed3bf7d00ee21705949a3b143ad245240/third_party/WebKit/Source/core/html/HTMLMarqueeElement.js
[modify] https://crrev.com/a95f76a0318a32bf5bf1153fb291402806c298d1/third_party/WebKit/public/blink_resources.grd

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e00d09a950c2a264e3e53fd0b40c7c7727175fb

commit 1e00d09a950c2a264e3e53fd0b40c7c7727175fb
Author: adithyas <adithyas@chromium.org>
Date: Wed Dec 07 18:06:23 2016

Use presentation attributes in HTMLMarquee

- Override isPresentationAttribute and collectStyleForPresentationAttribute
- Move "width: -webkit-fill-available" to UA stylesheet as styles set in shadow root have a higher precedence over presentation attribute styles
- Remove attributeChanged & attributeChangedCallback

BUG= 669656 

Review-Url: https://codereview.chromium.org/2554573005
Cr-Commit-Position: refs/heads/master@{#437004}

[modify] https://crrev.com/1e00d09a950c2a264e3e53fd0b40c7c7727175fb/third_party/WebKit/Source/core/css/html.css
[modify] https://crrev.com/1e00d09a950c2a264e3e53fd0b40c7c7727175fb/third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp
[modify] https://crrev.com/1e00d09a950c2a264e3e53fd0b40c7c7727175fb/third_party/WebKit/Source/core/html/HTMLMarqueeElement.h

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc9cbaf3d7f45808a4cead119b804296b6edf1a8

commit dc9cbaf3d7f45808a4cead119b804296b6edf1a8
Author: adithyas <adithyas@chromium.org>
Date: Fri Dec 09 17:03:15 2016

HTMLMarquee cleanup

- Follow up to http://crrev.com/2549443003
- Move RequestAnimationFrameCallback and AnimationComplete class
  definitions to .cpp file
- Prefix enums with k
- Change direction() and behavior() to getDirection() and getBehavior()
  respectively
- Use case insensitive match for direction and behavior
- Remove trueSpeed()
- Use String::numberToStringECMAScript to convert double to String instead of using String::format which is locale sensitive

BUG= 669656 

Review-Url: https://codereview.chromium.org/2554403002
Cr-Commit-Position: refs/heads/master@{#437569}

[modify] https://crrev.com/dc9cbaf3d7f45808a4cead119b804296b6edf1a8/third_party/WebKit/Source/core/html/HTMLMarqueeElement.cpp
[modify] https://crrev.com/dc9cbaf3d7f45808a4cead119b804296b6edf1a8/third_party/WebKit/Source/core/html/HTMLMarqueeElement.h

Status: Fixed (was: Started)

Comment 11 by tkent@chromium.org, Dec 14 2016

Blocking: 455596

Sign in to add a comment