1370705 - Move side effects of setting attribute of HTMLImageElement to after the attribute is set

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Robin Steuber (she/her) [:bytesized]]

The side effects [1] of setting attributes of HTMLImageElement should really happen after the attribute is set rather than before. This will be more important when Bug 1365092 lands, as then moving those side effects should allow the removal of the member variable HTMLImageElement::mForceReload which will be added in that patch.

[1] http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/dom/html/HTMLImageElement.cpp#525-580

Comment 1

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1370705 - Move attribute change effects from HTMLImageElement::BeforeMaybeChangeAttr to HTMLImageElement::AfterMaybeChangeAttr

It logically makes more sense for these effects to happen after the attribute has actually been changed and moving them allows us to get rid of the member variable HTMLImageElement::mForceReload.

Review commit: https://reviewboard.mozilla.org/r/147324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147324/

Comment 2

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8875920 [details]
Bug 1370705 - Move attribute change effects from HTMLImageElement::BeforeMaybeChangeAttr to HTMLImageElement::AfterMaybeChangeAttr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147324/diff/1-2/

Comment 3

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8875920 [details]
Bug 1370705 - Move attribute change effects from HTMLImageElement::BeforeMaybeChangeAttr to HTMLImageElement::AfterMaybeChangeAttr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147324/diff/2-3/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8875920 [details]
Bug 1370705 - Move attribute change effects from HTMLImageElement::BeforeMaybeChangeAttr to HTMLImageElement::AfterMaybeChangeAttr

https://reviewboard.mozilla.org/r/147324/#review152168

r=me

::: dom/html/HTMLImageElement.cpp:514
(Diff revision 3)
>        mNewRequestsWillNeedAnimationReset = true;
>  
>        // Force image loading here, so that we'll try to load the image from
>        // network if it's set to be not cacheable...  If we change things so that
>        // the state gets in Element's attr-setting happen around this
>        // LoadImage call, we could start passing false instead of aNotify

I think in the value changed case this is already the case.  But in the "same value" case it might not be... the comment might need updating accordingly.

Comment 5

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8875920 [details]
Bug 1370705 - Move attribute change effects from HTMLImageElement::BeforeMaybeChangeAttr to HTMLImageElement::AfterMaybeChangeAttr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147324/diff/3-4/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b1ae7752ab4
Move attribute change effects from HTMLImageElement::BeforeMaybeChangeAttr to HTMLImageElement::AfterMaybeChangeAttr r=bz

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b1ae7752ab4