Closed
Bug 1180991
Opened 9 years ago
Closed 9 years ago
[e10s] Page Info: Media preview is not shown the first time on multi-process Nightly
Categories
(Firefox :: Page Info Window, defect)
Tracking
()
VERIFIED
FIXED
Firefox 44
People
(Reporter: magicp.jp, Assigned: mconley)
References
Details
(Whiteboard: [bugday-20151104])
Attachments
(5 files)
User Agent: Mozilla/5.0 (Windows NT 6.3; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150706030206
Steps to reproduce:
1) Run Nightly 42.0a1 with enable multi-process
2) Open Nightly Start Page
3) Right click and select "View Page Info"
4) Select "Media" Pane
5) Select one by one a image
6) Media preview is not shown a image in the first time click
Actual results:
Media preview is not shown a image in the first time click. However, after move the other image, the second click works well.
Disable multi-process: It works fine!
Expected results:
Media preview should be shown a image in the e10s window.
Confirmed on...
- Windows 8.1 32bit
- Windows 10 Pro Insider Preview 64bit (Build 10162)
- Ubuntu 14.04 64bit
- Mac OS X Yosemite 10.10.4 64bit
Component: Untriaged → Page Info Window
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86
Updated•9 years ago
|
Comment 3•9 years ago
|
||
I think this is e10s implementation level bug. And reproduced since landing Bug 996785.
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=885e99ddd324&tochange=4f0a7452db01
Before landing Bug 996785 : No media pane appears
After landing Bug 996785 : Media pane appears, but Media preview does not appear in the first click of the list items.
Blocks: 996785
Updated•9 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 4•9 years ago
|
||
Hey jimicy - is your patch going to fix this?
Flags: needinfo?(jimmyw22)
Comment 5•9 years ago
|
||
magicp could you provide the website that you used to test this?
Better yet, would you be able to provide a video recording?
Using this page as an example
Open a private browsing window (calling page info on a page again has a different behavior on image preview. They all display)
Then
Select one by one a image
By using the down arrow to go through them
But I do get a media preview for the images on their first preview.
Only the first image doesn't make a first preview when you switch to the media tab.
First image doesn't make a first preview in release firefox either when you first open page info and select media tab. The image preview displays after switching to another image and then back which you did state.
Flags: needinfo?(jimmyw22) → needinfo?(magicp.jp)
Comment 6•9 years ago
|
||
(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #5)
> magicp could you provide the website that you used to test this?
> Better yet, would you be able to provide a video recording?
any page. e.g https://wiki.mozilla.org/Main_Page
>
> Using this page as an example
> Open a private browsing window (calling page info on a page again has a
> different behavior on image preview. They all display)
I can reproduce the problem regardless of private browsing mode
>
> Then
> Select one by one a image
> By using the down arrow to go through them
> But I do get a media preview for the images on their first preview.
>
No image is shown at first select in most cases
> Only the first image doesn't make a first preview when you switch to the
> media tab.
> First image doesn't make a first preview in release firefox either when you
> first open page info and select media tab. The image preview displays after
> switching to another image and then back which you did state.
This should be another bug.
Please find attached file "Bug 1180991 - Step to reproduce"
Flags: needinfo?(magicp.jp)
Updated•9 years ago
|
Assignee: mconley → jimmyw22
Comment 8•9 years ago
|
||
(In reply to magicp from comment #7)
> Created attachment 8630866 [details]
> Bug 1180991 - Step to reproduce.mp4
>
> Please find attached file "Bug 1180991 - Step to reproduce"
Thanks magicp! I see what you mean now.
I can also reproduce it. Image elements with the type background on the media tab don't have an image preview when you click on them the first time and have improperly sizes 0px x 0px the first time as well.
Whereas image elements of type image do have a image preview when you click on them the first time.
Updated•9 years ago
|
Comment 10•9 years ago
|
||
Hi Brad, yes I would like to continue to work on this.
Flags: needinfo?(jimmyw22)
Updated•9 years ago
|
Summary: [e10s] Page Info: Media preview is not shown on multi-process Nightly → [e10s] Page Info: Media preview is not shown the first time on multi-process Nightly
Assignee | ||
Updated•9 years ago
|
Assignee: jimmyw22 → mconley
Assignee | ||
Comment 11•9 years ago
|
||
Bug 1180991 - Send up natural dimensions of images loaded in content for Page Info. r?florian
Attachment #8669861 -
Flags: review?(florian)
Comment 12•9 years ago
|
||
Comment on attachment 8669861 [details]
MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian
https://reviewboard.mozilla.org/r/21273/#review19225
Random fact: with this patch you are editing the code I added in my very first patch in bug 229441.
::: browser/base/content/content.js:1162
(Diff revision 1)
> + let img = content.document.createElement("img");
What's the reason for creating a new element in the content document? Wouldn't item.natural\* give the same results?
Or do we need to do this only for background images?
::: browser/base/content/pageinfo/pageInfo.js:907
(Diff revision 1)
> - newImage.width = ("width" in item && item.width) || newImage.naturalWidth;
> + newImage.width = ("width" in item && item.width) || item.naturalWidth || newImage.naturalWidth;
Do we still have an edge case where we'll be setting the width and height to 0?
::: browser/base/content/pageinfo/pageInfo.js:913
(Diff revision 1)
> - newImage.width = newImage.naturalWidth;
> + newImage.width = item.naturalWidth || newImage.naturalWidth;
Do we expect to receive the natural\* values from the content process for background images?
Attachment #8669861 -
Flags: review?(florian)
Assignee | ||
Comment 13•9 years ago
|
||
https://reviewboard.mozilla.org/r/21273/#review19225
> What's the reason for creating a new element in the content document? Wouldn't item.natural\* give the same results?
>
> Or do we need to do this only for background images?
Yes - we want the natural dimensions for both images and background images, but background images need to be specially loaded like this so that we can get those dimensions.
> Do we still have an edge case where we'll be setting the width and height to 0?
I'm not sure - which edge case are you referring to?
> Do we expect to receive the natural\* values from the content process for background images?
With my patch, we do, yes.
Assignee | ||
Comment 14•9 years ago
|
||
Needinfo'ing for the edge case question in comment 13.
Flags: needinfo?(florian)
Comment 15•9 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> > Do we still have an edge case where we'll be setting the width and height to 0?
>
> I'm not sure - which edge case are you referring to?
Well, I guess if we have the image loaded neither in the content nor in the chrome process, we'll be setting the dimensions to 0 and the only way to fix that would be to change the dimensions from a load observer on the image tag in the page info dialog... but I guess that edge case existed before and isn't even really relevant, because Page Info is only supposed to be listing images that are shown in the page.
Flags: needinfo?(florian)
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
>
> > > Do we still have an edge case where we'll be setting the width and height to 0?
> >
> > I'm not sure - which edge case are you referring to?
>
> Well, I guess if we have the image loaded neither in the content nor in the
> chrome process, we'll be setting the dimensions to 0 and the only way to fix
> that would be to change the dimensions from a load observer on the image tag
> in the page info dialog... but I guess that edge case existed before and
> isn't even really relevant, because Page Info is only supposed to be listing
> images that are shown in the page.
That's correct, this bug exists already without e10s and without my patch.
I can certainly file that, but do you think it prevents us from landing this?
Flags: needinfo?(florian)
Comment 17•9 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #16)
> do you think it prevents us from landing this?
No.
The reason for not r+'ing the current patch was that I expect the createElement("img") thing to either be done only for background images (preferred), or a comment to be added to explain why we are doing it (ie. because the information is known for some images, but not all at that point). Actually, unless I'm confused, the |item instanceof Ci.nsIImageLoadingContent| test in the current patch seems to avoid creating the img element for background images and only do it for existing <img> tags.
I'm not even sure the edge case discussed above is worth filing a bug (we won't get to fixing it, right?). Maybe just rephrase a little bit the end of the comment, ie. "we'll assume that we can use the natural dimensions of the newImage we just created." should imply that when the natural dimensions of the newImage aren't known, then the image is mostly likely broken so dimensions don't matter.
Flags: needinfo?(florian)
Assignee | ||
Comment 18•9 years ago
|
||
https://reviewboard.mozilla.org/r/21273/#review19225
> I'm not sure - which edge case are you referring to?
Alright, dropping this as per https://bugzilla.mozilla.org/show_bug.cgi?id=1180991#c17
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8669861 [details]
MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian
Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian
Attachment #8669861 -
Attachment description: MozReview Request: Bug 1180991 - Send up natural dimensions of images loaded in content for Page Info. r?florian → MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian
Attachment #8669861 -
Flags: review?(florian)
Updated•9 years ago
|
Attachment #8669861 -
Flags: review?(florian) → review+
Comment 20•9 years ago
|
||
Comment on attachment 8669861 [details]
MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian
https://reviewboard.mozilla.org/r/21273/#review19693
Assignee | ||
Comment 21•9 years ago
|
||
Thanks, florian!
Assignee | ||
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a12d60c18af96fbd0a5064b8a1daf2adb3476b20
Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r=florian
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Comment 24•9 years ago
|
||
Reproduced this bug on Nightly 42.0a1 (2015-07-06) (Build ID: 20150706030206) by following Comment 0's instruction on Linux,64 bit
This Bug is now verified as fixed on Latest Firefox Nightly 44.0a1 (2015-10-15)
Build ID: 20151015030233
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
QA Whiteboard: [bugday-20151014]
Reporter | ||
Comment 25•9 years ago
|
||
Still reproduce in the latest Nightly 44.0a1
Please find attached movie
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•9 years ago
|
||
Are you able to reproduce on non about: pages?
If not, I suspect this is a distinct bug that we should file separately.
Flags: needinfo?(magicp.jp)
Reporter | ||
Comment 27•9 years ago
|
||
Yes, reproduce at https://addons.mozilla.org
Please find attached movie
Flags: needinfo?(magicp.jp)
Assignee | ||
Comment 28•9 years ago
|
||
Okay, confirmed - I'm seeing that too. I'll investigate this tomorrow.
Flags: needinfo?(mconley)
Assignee | ||
Comment 29•9 years ago
|
||
It looks like when background images are in block that's not been displayed that the images need to be loaded, and that occurs asynchronously. By the time the naturalWidth / naturalHeight is computed, the serialization of the node has already occurred.
So the problem that magicp is running into is a special-case of this bug.
I'm going to mark this bug RESOLVED again, and open a new bug for this special-case which can be fixed separately.
Flags: needinfo?(mconley)
Assignee | ||
Comment 30•9 years ago
|
||
Filed bug 1215589 for the background image bug.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 31•9 years ago
|
||
I have reproduced this bug with Firefox Nightly 42.0a1 (Build ID: 20150706030206) on
windows 8.1 64-bit with the instructions from comment 0 .
Verified as fixed with Firefox Nightly 45.0a1 (Build ID: 20151105030433)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
As it is also verified on Linux (Comment 24), I am marking this bug verified.
Status: RESOLVED → VERIFIED
Whiteboard: [bugday-20151104]
Assignee | ||
Comment 32•8 years ago
|
||
bugnotes |
You need to log in
before you can comment on or make changes to this bug.
Description
•