Last Comment Bug 848291 - Update TransitionEvent to be compatible with the spec
: Update TransitionEvent to be compatible with the spec
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla23
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on: 847599
Blocks: 868751
  Show dependency treegraph
 
Reported: 2013-03-06 03:56 PST by Olli Pettay [:smaug]
Modified: 2013-08-28 05:35 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (17.16 KB, patch)
2013-05-03 12:43 PDT, Olli Pettay [:smaug]
dbaron: review+
Details | Diff | Splinter Review
removed the comment in test (17.83 KB, patch)
2013-05-04 07:40 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
additional patch for the test (871 bytes, patch)
2013-05-04 08:23 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2013-03-06 03:56:14 PST
Better to do this after we have webidl bindings
Comment 1 Olli Pettay [:smaug] 2013-05-03 12:43:14 PDT
Created attachment 745299 [details] [diff] [review]
patch

This seems to work.
Note, the default values for the dictionary aren't in the spec, but by adding
default values, dictionary handling becomes a bit simpler in C++.

https://tbpl.mozilla.org/?tree=Try&rev=c355dd741e27
Comment 2 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-03 14:18:55 PDT
(In reply to Olli Pettay [:smaug] from comment #1)
> Note, the default values for the dictionary aren't in the spec, but by adding

Should they be?
Comment 3 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-03 14:28:14 PDT
Comment on attachment 745299 [details] [diff] [review]
patch

In test_transitions_events.html, you should fix:

>// We should get no events from transitions on pseudo-elements.
>$("seven").setAttribute("foo", "bar");

in two ways:

 (1) fix the comment
 (2) add two started_test() calls before it, since it is now expected to yield two events.  Otherwise the test will return before the last 2 tests finish.



I also think we should just remove initTransitionEvent, though at this point that should go in a separate patch.  Can you do that?
Comment 4 Olli Pettay [:smaug] 2013-05-04 07:40:43 PDT
Created attachment 745550 [details] [diff] [review]
removed the comment in test
Comment 5 Olli Pettay [:smaug] 2013-05-04 08:21:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1fac3f4bd83

but now I realized that perhaps I misunderstood the comment "in two ways"
Comment 6 Olli Pettay [:smaug] 2013-05-04 08:23:57 PDT
Created attachment 745554 [details] [diff] [review]
additional patch for the test
Comment 8 Olli Pettay [:smaug] 2013-05-04 10:16:56 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #2)
> (In reply to Olli Pettay [:smaug] from comment #1)
> > Note, the default values for the dictionary aren't in the spec, but by adding
> 
> Should they be?
IMO it would make the spec easier to read.
Comment 9 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-04 10:43:22 PDT
(In reply to Olli Pettay [:smaug] from comment #8)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #2)
> > (In reply to Olli Pettay [:smaug] from comment #1)
> > > Note, the default values for the dictionary aren't in the spec, but by adding
> > 
> > Should they be?
> IMO it would make the spec easier to read.

Does it change what the spec means?  It's not clear to me what's supposed to happen if a dictionary member without a default value is omitted.
Comment 10 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-04 10:47:02 PDT
I suppose if there's no default value, the spec is supposed to define what happens when it's not present.  (I don't see this happening much, though.)
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-04 10:49:43 PDT
Though it's not clear to me if giving the default values says anything about what happens when the entire dict is omitted.
Comment 13 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-06 17:24:57 PDT
spec fixed in https://dvcs.w3.org/hg/csswg/rev/172f4f12f059
Comment 14 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-06 17:33:28 PDT
... and https://dvcs.w3.org/hg/csswg/rev/3ee3ec0b5a5b

Note You need to log in before you can comment on or make changes to this bug.