Bug 55829 – Add information about how the value changed to Element::attributeChanged()

• Status: NEW
• Component: DOM

[Patrick R. Gansterer]

Add iformation about how the value changed to Element::attributeChanged()

[Patrick R. Gansterer]

Created attachment 84877 [details]
Patch

[Patrick R. Gansterer]

bug 55675 will use this additional argument.

[Alexey Proskuryakov]

What kind of performance testing did you do for this patch?

[Patrick R. Gansterer]

Created attachment 84890 [details]
PerformanceTest

(In reply to comment #3)
> What kind of performance testing did you do for this patch?
I did some test with the attached page (place it into PerformanceTests/parser), but I didn't get 100% reproducible results.
After the patch setAttribute of input element (seams to be the slowest) is sometimes about 1% slower or a little bit faster than the original version.
I also tried a version where I pass the old value as const AtomicString& instead of the AttributeChangedType, but it shows very similar results.
Can you try this patch on your machine too to verify this?

[Patrick R. Gansterer]

Comment on attachment 84877 [details]
Patch

I've done the performance testing as requested on IRC:

I'v run the dromaeo DOM and jslib test 3 times _without_ the patch:
http://dromaeo.com/?id=132284
http://dromaeo.com/?id=132285
http://dromaeo.com/?id=132287

The same test 3 times _with_ the patch:
http://dromaeo.com/?id=132316
http://dromaeo.com/?id=132318
http://dromaeo.com/?id=132324

Peacekeeper test _without_ the patch:
http://clients.futuremark.com/peacekeeper/results.action?key=5NoO (=4911)
http://clients.futuremark.com/peacekeeper/results.action?key=5Nof (=4891)
http://clients.futuremark.com/peacekeeper/results.action?key=5Nos (=4999)

Peacekeeper test _with_ the patch:
http://clients.futuremark.com/peacekeeper/results.action?key=5NqO (5009)
http://clients.futuremark.com/peacekeeper/results.action?key=5NqU (4989)
http://clients.futuremark.com/peacekeeper/results.action?key=5Nqc (4911)

I don't see a real difference.

cq-, because of a typo, thx to thakis

[Dave Hyatt]

I really don't like this approach.  It's not clear to me why this needs to be baked in at a low level.  Can't individual attributes just query for equivalence if they care?

[Dave Hyatt]

I think a better approach would be to not send a change notification at all when the value is the same, and just special case the small # of attributes that do care about receiving a change (basically URL attributes).

[Adam Roben (:aroben)]

Comment on attachment 84877 [details]
Patch

r- based on Hyatt's comments.

[Patrick R. Gansterer]

Created attachment 91803 [details]
Patch

[Eric Seidel (no email)]

It seems strange that Element has this knowledge.

[Patrick R. Gansterer]

Created attachment 93546 [details]
Patch

[Patrick R. Gansterer]

ping?

[Dave Hyatt]

Comment on attachment 93546 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93546&action=review

Giving r=me, but avoid duplicating that code.

> Source/WebCore/dom/Element.cpp:690
> +        bool sameValue = old->value() == value;
> +        if (!sameValue) {
> +            if (Attr* attrNode = old->attr())
> +                attrNode->setValue(value);
> +            else
> +                old->setValue(value);
> +        }
> +        if (!sameValue || fireEveryAttributeChange(old))
> +            attributeChanged(old);

Pull this into a little static helper please to avoid duplication.

[Patrick R. Gansterer]

Created attachment 97383 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 97383 [details]
Patch for landing

Rejecting attachment 97383 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:

=> Tests to be fixed (1224):
      6 DumpRenderTree crashes   ( 0.5%)
      5 tests timed out          ( 0.4%)
     51 text diff mismatch       ( 4.2%)
    328 skipped                  (26.8%)

=> Tests that will only be fixed if they crash (WONTFIX) (8116):
      1 test timed out           ( 0.0%)
    109 text diff mismatch       ( 1.3%)
   7957 skipped                  (98.0%)


Regressions: Unexpected text diff mismatch : (1)
  svg/animations/animateTransform-pattern-transform.html = TEXT



Full output: http://queues.webkit.org/results/8880017

[WebKit Review Bot]

Created attachment 97384 [details]
Archive of layout-test-results from ec2-cq-03

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.