Remove MediaQueryList listener from removed HTMLSourceElement.

Remove MediaQueryList listener from removed HTMLSourceElement.

Currently the listener is not removed when HTMLSourceElement is removed from the DOM,
resulting in needless downloads when viewport changes after such removal. This CL fixes that.

BUG=633875
Committed: https://crrev.com/dfdb63d9b535f4d20901a7e95f91bf503978ec2f
Cr-Commit-Position: refs/heads/master@{#416468}

[Yoav Weiss, 2016-08-16 22:37:26]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 22:38:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yoav Weiss, 2016-08-16 22:38:40]

yoav@yoav.ws changed reviewers:
+ cbiesinger@chromium.org

[Yoav Weiss, 2016-08-16 22:38:40]

Hey Christian :)

Been a while since we messed around with this code, but mind taking a stab at
reviewing?

[commit-bot: I haz the power, 2016-08-17 00:14:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 00:14:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[cbiesinger, 2016-08-17 19:32:05]

Well, here are some comments. I'm not sure I should be giving the final lgtm
though, I haven't been in this code forever...

Also, you need to fix the tests, maybe my changes below are sufficient.

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/loading/image-picture-no-download-after-removal.html
(right):

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/loading/image-picture-no-download-after-removal.html:2:
<script src="../resources/testharness.js"></script>
I thought testharness required a <div id="log"></div> in the document, though I
could be wrong

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/loading/resources/image-picture-no-download-after-picture-removal-frame.html
(right):

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/loading/resources/image-picture-no-download-after-picture-removal-frame.html:5:
document.body.removeChild(document.getElementsById("pic"));
Elements -> Element

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/loading/resources/image-picture-no-download-after-source-removal-frame.html
(right):

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/loading/resources/image-picture-no-download-after-source-removal-frame.html:5:
window.pic.removeChild(window.src);
I think you want window.src -> document.getElementByid('src')...?

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:405:
m_source->notifyImgSiblingRemoved();
What about the <source> elements that were not chosen? Shouldn't they also
remove their listeners?

And... when an <img> element appears again, don't they need to re-add their
listeners?

[Yoav Weiss, 2016-08-30 07:37:07]

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/loading/image-picture-no-download-after-removal.html
(right):

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/loading/image-picture-no-download-after-removal.html:2:
<script src="../resources/testharness.js"></script>
On 2016/08/17 19:32:05, cbiesinger wrote:
> I thought testharness required a <div id="log"></div> in the document, though
I
> could be wrong

I don't think that's necessary

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/loading/resources/image-picture-no-download-after-picture-removal-frame.html
(right):

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/loading/resources/image-picture-no-download-after-picture-removal-frame.html:5:
document.body.removeChild(document.getElementsById("pic"));
On 2016/08/17 19:32:05, cbiesinger wrote:
> Elements -> Element

yeah, that test is all kinds of broken. should be fixed now

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/loading/resources/image-picture-no-download-after-source-removal-frame.html
(right):

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/loading/resources/image-picture-no-download-after-source-removal-frame.html:5:
window.pic.removeChild(window.src);
On 2016/08/17 19:32:05, cbiesinger wrote:
> I think you want window.src -> document.getElementByid('src')...?

changed

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/2256533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:405:
m_source->notifyImgSiblingRemoved();
On 2016/08/17 19:32:05, cbiesinger wrote:
> What about the <source> elements that were not chosen? Shouldn't they also
> remove their listeners?
> 
> And... when an <img> element appears again, don't they need to re-add their
> listeners?

Good call. I'll add that.

[Yoav Weiss, 2016-08-30 07:39:52]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 07:40:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 09:18:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 09:18:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yoav Weiss, 2016-09-01 13:17:22]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 13:17:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yoav Weiss, 2016-09-01 13:21:25]

yoav@yoav.ws changed reviewers:
+ foolip@chromium.org

[Yoav Weiss, 2016-09-01 13:21:25]

foolip@ - care to take a look?

(git blamed HTMLSourceElement.cpp and saw that you messed around with it in the
past :D)

[commit-bot: I haz the power, 2016-09-01 14:40:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 14:40:24]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-09-01 14:45:14]

I believe I've poked at this file before, but not as it relates to <picture>. I
have a bandwidth problem right now, care to find another reviewer for this?
Sorry :/

[Yoav Weiss, 2016-09-01 14:46:12]

On 2016/09/01 14:45:14, foolip wrote:
> I believe I've poked at this file before, but not as it relates to <picture>.
I
> have a bandwidth problem right now, care to find another reviewer for this?
> Sorry :/

sure, np :)

[Yoav Weiss, 2016-09-01 15:07:50]

cbiesinger@ - will you be willing to take another look? I think that even though
you haven't touched that code recently, you're still the best reviewer, as no
one else has touched it since.

[cbiesinger, 2016-09-02 19:09:22]

OK then -- lgtm, see comments below. And you need to fix the red try servers!

https://codereview.chromium.org/2256533002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/loading/image-picture-download-after-shrink.html
(right):

https://codereview.chromium.org/2256533002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/image-picture-download-after-shrink.html:5:
var t = async_test('Make sure that previousely non-matching source based image
resources are downloaded after the viewport changes.');
previousely -> previously

https://codereview.chromium.org/2256533002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLSourceElement.cpp (right):

https://codereview.chromium.org/2256533002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLSourceElement.cpp:84: if
(media.isEmpty())
By the way -- I'm wondering if we should still reset m_mediaQueryList and remove
the listener if this is true? Anyway -- that's unrelated to this patch.

[Yoav Weiss, 2016-09-04 04:03:15]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-04 04:03:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yoav Weiss, 2016-09-04 04:15:52]

Thanks for reviewing!

https://codereview.chromium.org/2256533002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLSourceElement.cpp (right):

https://codereview.chromium.org/2256533002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLSourceElement.cpp:84: if
(media.isEmpty())
On 2016/09/02 19:09:22, cbiesinger wrote:
> By the way -- I'm wondering if we should still reset m_mediaQueryList and
remove
> the listener if this is true? Anyway -- that's unrelated to this patch.

I think you're right and that's another bug that would trigger spurious
downloads on resize. Good catch! :)

[commit-bot: I haz the power, 2016-09-04 07:09:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-04 07:09:29]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-09-04 07:29:59]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2016-09-04 07:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2256533002/#ps60001
(title: "typo")

[commit-bot: I haz the power, 2016-09-04 07:30:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-04 07:35:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-04 07:38:17]

Description was changed from

==========
Remove MediaQueryList listener from removed HTMLSourceElement.

Currently the listener is not removed when HTMLSourceElement is removed from the
DOM,
resulting in needless downloads when viewport changes after such removal. This
CL fixes that.

BUG=633875
==========

to

==========
Remove MediaQueryList listener from removed HTMLSourceElement.

Currently the listener is not removed when HTMLSourceElement is removed from the
DOM,
resulting in needless downloads when viewport changes after such removal. This
CL fixes that.

BUG=633875

Committed: https://crrev.com/dfdb63d9b535f4d20901a7e95f91bf503978ec2f
Cr-Commit-Position: refs/heads/master@{#416468}
==========

[commit-bot: I haz the power, 2016-09-04 07:38:18]

Patchset 4 (id:??) landed as
https://crrev.com/dfdb63d9b535f4d20901a7e95f91bf503978ec2f
Cr-Commit-Position: refs/heads/master@{#416468}