Closed Bug 1302513 Opened 8 years ago Closed 7 years ago

Deprecate (remove?) getAuthoredPropertyValue()

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox57 --- fixed

People

(Reporter: dholbert, Assigned: bradwerth)

References

Details

(Keywords: addon-compat)

Attachments

(5 files, 1 obsolete file)

Bug 1302513 Part 1: Remove test for getAuthoredPropertyValue API (since that API is being
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1302513 Part 2: Remove declaration of getAuthoredPropertyValue from webidl.
59 bytes, text/x-review-board-request
dholbert
: review+
bholley
: review+
Details
Bug 1302513 Part 3: Remove declarations and implementations of getAuthoredPropertyValue.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1302513 Part 4: Remove the nsCSSValue::Serialization eAuthorSpecified enum, which is no longer used.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1302513 Part 5: Simplify nsCSSValue::AppendToString, now that aSerialization can only take one value.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
In bug 731271, we added a function CSSStyleDeclaration.getAuthoredPropertyValue() which is supposed to return a serialization that's closer to what the author specified. The goal was for it to be used in devtools panels. In practice, though, this is the state of the world: (1) devtools does not currently call this function at all (and I'm not clear if it ever did) (2) The function isn't particularly useful right now; it's identical to getPropertyValue(), *except* that it serializes author-specified "rgba(RRR, GGG, BBB, 1)" using that 4-component syntax as opposed to the shorter & equivalent "rgb(RRR, GGG, BBB)" (3) To the extent that this difference is useful, it's becoming less useful -- because in css-color-4, "rgb" and "rgba" use identical syntax, and both accept 3-4 components. (And technically "rgba" is now becoming a "legacy" alias, with rgb() being the canonical shorter form.) So: as we implement css-color-4 in bug 1295456, we're left with a question of what (if anything) special we should do for getAuthoredPropertyValue(), in this new world where rgb & rgba use identical syntax. Is it really useful for us to care whether authors used "rgb" vs. "rgba"? I suspect we should just remove this special case and deprecate getAuthoredPropertyValue(), and then remove support for it once we've confirmed that nothing depends on it anymore. (Right now Firebug uses it, according to https://dxr.mozilla.org/addons/search?q=getAuthoredPropertyValue , but as noted above, I'm skeptical that Firebug is getting much value out of it.)
Michael, you filed bug 731271, and you were involved with the design & requirements of this feature. Do you know if it ever got used (aside from in Firebug), and do you have any other context or thoughts about potential-deprecation?
Blocks: 731271
Flags: needinfo?(mratcliffe)
(Also tagging Jan for his thoughts on this, since Firebug seems to be the sole user of this function. Specifically: is this function actually serving a useful purpose in Firebug, given that it only differs from getPropertyValue on this one subtle rgb/rgba point? And do you have any concerns about it being deprecated [and ultimately removed], particularly given that rgb/rgba will soon be aliases?)
Flags: needinfo?(odvarko)
For reference, here's the css-color-4 spec's section on the new 3-or-4 component rgb() syntax. rgba gets documented as an afterthought now: > ... for legacy reasons, an rgba() function also exists, with an > identical grammar and behavior to rgb(). https://drafts.csswg.org/css-color-4/#funcdef-rgb
I don't believe we ever used it because Tom Tromey (tromey) exposed the CSS parser to JS giving us much more information than getAuthoredPropertyValue() could ever give us.
Flags: needinfo?(mratcliffe)
OK, cool. (Maybe that's the API that Firebug really should be using here, instead of getAuthoredPropertyValue? Do you or tromey know where we'd want to direct Firebug folks, if they wanted to learn more about that as a replacement for getAuthoredPropertyValue?)
(In reply to Daniel Holbert [:dholbert] from comment #5) > OK, cool. (Maybe that's the API that Firebug really should be using here, > instead of getAuthoredPropertyValue? Do you or tromey know where we'd want > to direct Firebug folks, if they wanted to learn more about that as a > replacement for getAuthoredPropertyValue?) Basically we have the platform CSS lexer available (now rewritten into JS in support of the html-ification project) and we use that, plus some code in the style actors that lets us fetch the rule text, to rewrite rules in situ, using the rewriter in css-parsing-utils. There are a lot of moving parts so it is difficult to summarize; but if someone wants to give it a go I would be happy to go over it all.
(In reply to Daniel Holbert [:dholbert] from comment #2) > (Also tagging Jan for his thoughts on this, since Firebug seems to be the > sole user of this function. > > Specifically: is this function actually serving a useful purpose in Firebug, > given that it only differs from getPropertyValue on this one subtle rgb/rgba > point? And do you have any concerns about it being deprecated [and > ultimately removed], particularly given that rgb/rgba will soon be aliases?) Agree, I think it's ok to deprecated and remove it eventually. Also, Firebug is checking whether this function exists and it's using different code path if not. So, this should help to adopt new world state. Honza
Flags: needinfo?(odvarko)
Priority: -- → P3
See Also: → 1348165
Assignee: nobody → bwerth
Blocks: 1383296
Attachment #8888927 - Flags: review?(dholbert)
Attachment #8888928 - Flags: review?(dholbert)
Attachment #8888929 - Flags: review?(dholbert)
Attachment #8888930 - Flags: review?(dholbert)
Comment on attachment 8888927 [details] Bug 1302513 Part 1: Remove test for getAuthoredPropertyValue API (since that API is being https://reviewboard.mozilla.org/r/159958/#review165340 r=me with commit message clarified as follows ::: commit-message-85429:1 (Diff revision 1) > +Bug 1302513 Part 1: Remove a test that calls getAuthoredPropertyValue. Let's clarify this to: "Remove test for getAuthoredPropertyValue API (which is being deprecated)" because: (1) The current commit message sounds like it's talking about a "test that just happens to call getAuthoredPropertyValue" -- and if that were true, then it'd be overkill to remove the whole test. (we'd just want to remove that one call, and leave the rest of the test intact). But really this is a test *for* the getAuthoredPropertyValue API, so removal makes sense if we're removing that API. (2) it's nice to include a hint at the "why" in commit messages (hence the parenthetical)
Attachment #8888927 - Flags: review?(dholbert) → review+
Comment on attachment 8888927 [details] Bug 1302513 Part 1: Remove test for getAuthoredPropertyValue API (since that API is being https://reviewboard.mozilla.org/r/159958/#review165340 > Let's clarify this to: > "Remove test for getAuthoredPropertyValue API (which is being deprecated)" > > because: > (1) The current commit message sounds like it's talking about a "test that just happens to call getAuthoredPropertyValue" -- and if that were true, then it'd be overkill to remove the whole test. (we'd just want to remove that one call, and leave the rest of the test intact). But really this is a test *for* the getAuthoredPropertyValue API, so removal makes sense if we're removing that API. > > (2) it's nice to include a hint at the "why" in commit messages (hence the parenthetical) > Let's clarify this to: > "Remove test for getAuthoredPropertyValue API (which is being deprecated)" Or maybe even more accurate: "Remove test for getAuthoredPropertyValue API (since that API is being removed)"
Comment on attachment 8888928 [details] Bug 1302513 Part 2: Remove declaration of getAuthoredPropertyValue from webidl. https://reviewboard.mozilla.org/r/159960/#review165348 I initially suspected that this might not build on its own until we've also got part 3. But I just tried building this locally and proved my suspicion wrong. :) So this seems good.
Attachment #8888928 - Flags: review?(dholbert) → review+
Comment on attachment 8888929 [details] Bug 1302513 Part 3: Remove declarations and implementations of getAuthoredPropertyValue. https://reviewboard.mozilla.org/r/159962/#review165360 r=me
Attachment #8888929 - Flags: review?(dholbert) → review+
Comment on attachment 8888930 [details] Bug 1302513 Part 4: Remove the nsCSSValue::Serialization eAuthorSpecified enum, which is no longer used. https://reviewboard.mozilla.org/r/159964/#review165362 r=me
Attachment #8888930 - Flags: review?(dholbert) → review+
Attachment #8888991 - Attachment is obsolete: true
Attachment #8888998 - Flags: review?(dholbert)
Comment on attachment 8888998 [details] Bug 1302513 Part 5: Simplify nsCSSValue::AppendToString, now that aSerialization can only take one value. https://reviewboard.mozilla.org/r/160030/#review165446 r=me
Attachment #8888998 - Flags: review?(dholbert) → review+
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 5 changesets with 13 changes to 13 files remote: remote: WebIDL file dom/webidl/CSSStyleDeclaration.webidl altered in changeset 1cc0c27c9cd1 without DOM peer review remote: remote: remote: remote: ************************** ERROR **************************** remote: remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=... remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding.. remote: remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.d_webidl hook failed abort: push failed on remote
Attachment #8888928 - Flags: review?(cam)
Cameron isn't a DOM peer, so his review won't help here. You need someone from https://wiki.mozilla.org/Modules/All#Document_Object_Model (some of the folks there are inactive, but perhaps mrbkap or bholley would be a good choice. Or bz, though I think he's not accepting new review requests at the moment.) You also should send an "intent to un-ship" to the dev-platform mailing list: https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_unship Given that this was marked as ChromeOnly in the WebIDL (i.e. not exposed to the web), it should be pretty uncontroversial; but still worth reporting that the API is going away to folks who care about Gecko APIs.
Flags: needinfo?(bwerth)
(I should've thought of the DOM peer review requirement & the intent-to-unship during code-review -- sorry for not thinking of those sooner.)
Attachment #8888928 - Flags: review?(cam) → review?(bobbyholley)
Comment on attachment 8888928 [details] Bug 1302513 Part 2: Remove declaration of getAuthoredPropertyValue from webidl. https://reviewboard.mozilla.org/r/159960/#review166836 Looks like this is used by firebug: https://dxr.mozilla.org/addons/search?q=getAuthoredPropertyValue&redirect=true Mind waiting a week until after the beta merge before landing this? That will mean it lands on 57 we won't have to worry about legacy addons.
Attachment #8888928 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #27) > Looks like this is used by firebug: > https://dxr.mozilla.org/addons/search?q=getAuthoredPropertyValue&redirect=true Firebug developers (Jan Honza Odvarko) are aware of & OK with this removal, I think -- see comment 2 & comment 7. Firebug has graceful fallback, it seems. ("Firebug is checking whether this function exists and it's using different code path if not.") > Mind waiting a week until after the beta merge before landing this? That > will mean it lands on 57 we won't have to worry about legacy addons. (That makes sense to me, I think.)
Comment on attachment 8888928 [details] Bug 1302513 Part 2: Remove declaration of getAuthoredPropertyValue from webidl. https://reviewboard.mozilla.org/r/159960/#review166836 Will do. We were told by honza in https://bugzilla.mozilla.org/show_bug.cgi?id=1302513#c7 that firebug merely checks for existence of this function and if it fails, uses a different codepath.
(In reply to Tom Tromey :tromey from comment #6) > (In reply to Daniel Holbert [:dholbert] from comment #5) > > OK, cool. (Maybe that's the API that Firebug really should be using here, > > instead of getAuthoredPropertyValue? Do you or tromey know where we'd want > > to direct Firebug folks, if they wanted to learn more about that as a > > replacement for getAuthoredPropertyValue?) > > Basically we have the platform CSS lexer available (now rewritten into JS in > support of the > html-ification project) and we use that, plus some code in the style actors > that lets us > fetch the rule text, to rewrite rules in situ, using the rewriter in > css-parsing-utils. > There are a lot of moving parts so it is difficult to summarize; but if > someone wants to give > it a go I would be happy to go over it all. Tom, it would be very helpful if you were willing to write a two paragraph how-to that would be suitable for use in the intent-to-unship announcement. Without your detail, my attempt would be very hand-wavy.
Flags: needinfo?(bwerth) → needinfo?(ttromey)
(In reply to Brad Werth [:bradwerth] from comment #30) > Tom, it would be very helpful if you were willing to write a two paragraph > how-to that would be suitable for use in the intent-to-unship announcement. > Without your detail, my attempt would be very hand-wavy. Code using getAuthoredPropertyValue can instead do what the DevTools do: fetch the original CSS style sheet, and then use the CSS lexer to tokenize it. The DevTools contains an ad hoc parser for CSS rules that could be extracted for more general use, if someone was motivated to do it. There are two chrome-only APIs in inIDOMUtils.idl that are useful here. 1. getRelativeRuleLine returns the line number of a CSS rule, relative to the <style> element that contains it. This can be useful to find the proper starting location to extract the text of a rule. 2. getCSSLexer returns a CSS lexer that will lex some text. The lexer is documented here: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CSSLexer.webidl
Flags: needinfo?(ttromey)
(In reply to Tom Tromey :tromey from comment #31) Thank you very much, Tom. Intent-to-unship e-mail has been sent containing much improved detail based on the information you provided.
Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a67ced4189ae Part 1: Remove test for getAuthoredPropertyValue API (since that API is being r=dholbert https://hg.mozilla.org/integration/autoland/rev/cf2bc277cf05 Part 2: Remove declaration of getAuthoredPropertyValue from webidl. r=bholley,dholbert https://hg.mozilla.org/integration/autoland/rev/336a4f9435ce Part 3: Remove declarations and implementations of getAuthoredPropertyValue. r=dholbert https://hg.mozilla.org/integration/autoland/rev/0bea7e0c1998 Part 4: Remove the nsCSSValue::Serialization eAuthorSpecified enum, which is no longer used. r=dholbert https://hg.mozilla.org/integration/autoland/rev/0fc9373062ae Part 5: Simplify nsCSSValue::AppendToString, now that aSerialization can only take one value. r=dholbert
We can probably remove aSerialization as well... but that probably needs a lot more work and may not be worth doing at this moment.
(In reply to Brad Werth [:bradwerth] from comment #32) > (In reply to Tom Tromey :tromey from comment #31) > > Thank you very much, Tom. Intent-to-unship e-mail has been sent containing > much improved detail based on the information you provided. Did your mail get stuck in moderation? I haven't seen it come through...
Flags: needinfo?(bwerth)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #34) > We can probably remove aSerialization as well... but that probably needs a > lot more work and may not be worth doing at this moment. Bug 1383296 addresses that. A patch is ready after this bug lands.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #35) > Did your mail get stuck in moderation? I haven't seen it come through... Yes, in moderation. I expect it to be accepted. Here's the text: > https://bugzilla.mozilla.org/show_bug.cgi?id=1302513 > > This chrome-only API was intended to assist developer tools in reporting the authored values for properties that are normalized after parsing. We are removing it for four reasons: > > 1) Only color properties were specially handled by this API. > 2) Firefox devtools doesn't call this API, and the only known add-on that does is Firebug, which has a fallback. > 3) There are APIs to do CSS lexing in JS, which provides another way to retrieve authored values. > 4) If this API is retained, the transition to Quantum Style aka "Stylo" would require significant code changes in the Quantum Style code and would introduce some performance inefficiency. > > If you need to return authored values, you can use a mthod similar to the one used by the Firefox devtools. There are two chrome-only APIs in inIDOMUtils.idl: > > 1) getCSSLexer returns a CSS lexer that will lex some text. The lexer is documented here: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CSSLexer.webidl > > 2) getRelativeRuleLine returns the line number of a CSS rule, relative to the <style> element that contains it. This can be useful to find the proper starting location to extract the text of a rule. > > If we proceed with unshipping, Firefox 57 will be the first release version affected. > > Feedback welcome, thank you, > > Brad Werth > Platform Engineer > bwerth@mozilla.com
Flags: needinfo?(bwerth)
Keywords: site-compat
"This chrome-only API"
Keywords: site-compat → addon-compat
This is Chrome-only (so doesn't need mentioning in the Fx rel notes), and we never documented it in the first place, so I don't think there's anything for me to do here. Removing ddn.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: