Closed
Bug 997101
Opened 11 years ago
Closed 10 years ago
[Gaia] Mediaquery in CSS may extend load time of app
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Tracking
()
People
(Reporter: gduan, Assigned: kanru)
References
Details
(Keywords: perf, regression, Whiteboard: [c=progress p= s= u=])
Attachments
(2 files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
kats
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Device: hamachi, 20140415160202 master-central build.
command: |b2gperf --delay=10 Settings|.
| Results for Settings, cold_load_time: median:1693, mean:1700, std: 29, max:1812, min:1670, all:1777,1677,1687,1701,1713,1703,1709,1732,1692,1674,1711,1682,1686,1672,1690,1723,1700,1678,1700,1683,1695,1670,1694,1682,1685,1812,1705,1692,1674,1705
After marking the mediaquery line (only /*@media(condition) {/* and /*}*/ ) of settings_large.css
| Results for Settings, cold_load_time: median:1356, mean:1371, std: 41, max:1493, min:1336, all:1401,1363,1339,1370,1353,1346,1353,1354,1358,1352,1350,1352,1359,1472,1412,1368,1365,1370,1338,1343,1345,1336,1361,1490,1355,1354,1371,1345,1367,1493
Reporter | ||
Updated•11 years ago
|
Component: Gaia → CSS Parsing and Computation
Product: Firefox OS → Core
Reporter | ||
Comment 1•11 years ago
|
||
Please let me know if any information is not cleared enough.
Reporter | ||
Comment 2•11 years ago
|
||
Hi David,
is the result expected? Could you advise and let me know if any information is unclear ? Thanks.
Flags: needinfo?(dbaron)
Comment 3•11 years ago
|
||
Is the document initially being laid out at the wrong size (such that the media query doesn't match) and then the size changes and we have to restyle because the media query starts matching?
Comment 4•11 years ago
|
||
according to https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/settings_large.css
The media query statement is
```
@media (min-width: 768px) and (orientation: portrait) {
```
The original css is target for phone size device (hamachi is one of them), so the media query is not affect the rendered style.
Comment 5•11 years ago
|
||
I think this is not expected. Does nsStyleSet::MediumFeaturesChanged ever return true during this test, and if so, why?
Flags: needinfo?(dbaron)
Comment 6•11 years ago
|
||
(Maybe bug 1001214 is related?)
Comment 7•11 years ago
|
||
David, forgive I have limit knowledge about internal implement, but I could help bring more test results to clarify the issue.
Following tests test settings app with command `b2gperf --delay=10 Settings`
base on 4/28 gaia master
cold_load_time: median:1636, mean:1648, std: 40
comment out all @media in https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/settings_large.css (no @media)
cold_load_time: median:1290, mean:1291, std: 17
only enable one plain @media (min-width: 768px) (one @media)
cold_load_time: median:1427, mean:1431, std: 28
only one @media (min-width: 768px) and (orientation: portrait)
cold_load_time: median:1411, mean:1413, std: 30,
** multiple @media (min-width: 768px) (3 plain @media)
cold_load_time: median:1634, mean:1646, std: 50
I saw if I put 3 @media (min-width: 768px) in settings_large.css, I got the same load_time as normal one (@media, @media with portrait, @media with landscape)...
Comment 9•11 years ago
|
||
Carrying over nomination from the dupe.
blocking-b2g: --- → 1.4?
Keywords: perf,
regression
Comment 10•11 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #7)
> I saw if I put 3 @media (min-width: 768px) in settings_large.css, I got the
> same load_time as normal one (@media, @media with portrait, @media with
> landscape)...
The number of media queries doesn't matter; the issue is that when one (or more) dynamically changes, we have to do a lot of work. (If more than one change, the work gets coalesced.)
I'd like to know the answer to comment 5.
Flags: needinfo?(gduan)
Flags: needinfo?(gasolin)
Comment 11•11 years ago
|
||
It's a pretty normal use case in mobile since @media will be used for different layout with portrait and landscape. (at least call twice)
Thinker, This issue increase 300ms load_time in settings.app and also impact other app that applied @media.
George and I are not familiar with gecko. Could you help find a man to check the answer of comment 5?
Flags: needinfo?(tlee)
Flags: needinfo?(gduan)
Flags: needinfo?(gasolin)
Assignee | ||
Comment 13•11 years ago
|
||
My test result with gecko(git) 177f1467fe65ccc8600208c17473f798118860d1 and gaia 1ba9199d69f4621b78556ab0ce716c641728dffa
* With media query, css rules
Results for Settings, cold_load_time: median:6016, mean:6072, std: 131, max:6461, min:5951, all:6277,6009,5991,5974,5966,6005,6087,6330,5951,6014,6033,6131,5980,6018,6000,5968,6059,6411,6201,6008,5998,5996,5972,6050,6044,5994,6102,6461,6116,6024
* With media query, css rules
Results for Settings, cold_load_time: median:6033, mean:6084, std: 141, max:6610, min:5947, all:6091,6125,6038,5988,6128,6091,6115,6405,5993,6027,6070,6035,5947,6020,6000,5977,6610,6218,6036,6036,6010,6030,6009,5985,6032,6019,6380,6093,6028,5991
* With media query
Results for Settings, cold_load_time: median:5480, mean:5543, std: 366, max:7417, min:5211, all:5625,5453,5443,5507,5211,5449,5339,7417,5803,5566,5518,5457,5483,5412,5500,5412,5404,5501,5596,5612,5478,5529,5452,5412,5216,5391,5420,5529,5606,5575
* empty
Results for Settings, cold_load_time: median:5160, mean:5400, std: 685, max:7562, min:4870, all:5172,5135,5162,5196,5138,5121,7496,5272,5367,5137,5136,5171,4870,5145,5137,5120,5256,7562,5367,5269,5158,5141,5146,5114,5141,5108,5164,5218,7248,5339
* With css rules
Results for Settings, cold_load_time: median:5073, mean:5289, std: 641, max:7290, min:4829, all:5112,5048,5049,5060,5052,7290,5019,4951,5215,5078,5080,4829,5062,4843,5094,5141,7079,5534,4975,5062,5052,5127,5090,5069,5032,4839,5109,7100,5559,5122
Blocks: 1002842
Comment 14•11 years ago
|
||
Blocking for 1.4 as this is needed by a blocker : https://bugzilla.mozilla.org/show_bug.cgi?id=1002842
blocking-b2g: 1.4? → 1.4+
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #15)
> comment 5?
Yes. `nsStyleSet::MediumFeaturesChanged' was called twice during start-up and all return true. It returned true because mRuleProcessors[3]->MediumFeaturesChanged(aPresContext) was true.
The first mRuleProcessors[3]->MediumFeaturesChanged(aPresContext) returned true because it created newCascade. The second call to mRuleProcessors[3]->MediumFeaturesChanged(aPresContext) returned true because it find one mCacheKey.Matches in the mRuleCascades.
Flags: needinfo?(kchen)
Assignee | ||
Comment 17•11 years ago
|
||
If I remove all the media queries, `nsStyleSet::MediumFeaturesChanged' was still called twice during start-up, but never return true.
Comment 18•11 years ago
|
||
I set assignee per Thinker's comment 12.
Assignee: nobody → kchen
Priority: -- → P1
Whiteboard: [c=progress p= s= u=]
Updated•11 years ago
|
Severity: normal → blocker
Status: NEW → ASSIGNED
Whiteboard: [c=progress p= s= u=] → [c=progress p= s= u=1.4]
Comment 19•11 years ago
|
||
And which media queries changed dynamically, and why?
Comment 20•11 years ago
|
||
Is the following kind of media query affected?
<link rel="stylesheet" type="text/css" href="style/video_tablet.css" media="(min-height: 768px) and (min-width: 768px)" />
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #19)
> And which media queries changed dynamically, and why?
http://hg.mozilla.org/mozilla-central/file/1204667a2935/dom/ipc/TabChild.cpp#l738
When TabChild receives before-first-paint, it sets the CSS viewport to a default value which triggers one MediumFeaturesChanged. It then sets CSS viewport again in HandlePossibleViewportChange which triggers again MediumFeaturesChanged.
Comment 22•11 years ago
|
||
tn: can you comment on this one? I think this code path is only reached with APZC. Can we avoid having this event fire twice at page load?
Flags: needinfo?(tnikkel)
Comment 23•11 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #21)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #19)
> > And which media queries changed dynamically, and why?
>
> http://hg.mozilla.org/mozilla-central/file/1204667a2935/dom/ipc/TabChild.
> cpp#l738
>
> When TabChild receives before-first-paint, it sets the CSS viewport to a
> default value which triggers one MediumFeaturesChanged. It then sets CSS
> viewport again in HandlePossibleViewportChange which triggers again
> MediumFeaturesChanged.
It looks like HandlePossibleViewportChange doesn't need anything from the new FrameMetrics it gets back from the APZC, it just asks the document for the viewport info via nsContentUtils::GetViewportInfo. So can we just do the same thing for before-first-paint instead of using a default use?
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 24•11 years ago
|
||
Kats, could we remove the first SetCSSViewport (linked in comment 21)?
Assignee: kchen → nobody
Flags: needinfo?(bugmail.mozilla)
Comment 25•11 years ago
|
||
Kanru, who do you think could be a good owner for this bug? Thanks.
Flags: needinfo?(kchen)
Comment 26•11 years ago
|
||
Talked with Kanru. If we could just remove what Kanru mentioned in #24, he can own it. Otherwise, we may need to find someone to help this bug. (Maybe Kats?)
Flags: needinfo?(kchen)
Comment 27•11 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #24)
> Kats, could we remove the first SetCSSViewport (linked in comment 21)?
We might be able to, but I suspect that it will break things. You're welcome to try it though, because I can't say that I can think of a specific case that will break. I just know that the code is very brittle and prone to breaking.
The main things I would test are making sure pages in the browser still load with the correct initial viewport. You should test pages with and without meta-viewport tags, as well as loading non-HTML documents (such as images) directly, and things like data URLs.
Flags: needinfo?(bugmail.mozilla)
Comment 28•11 years ago
|
||
Guys we need an owner for this, it's blocking the 1.4 release.
Kanru, looks like you're it per comment 12 and comment 27. Please go ahead with Kats' suggestions in comment 27.
Thanks,
Mike
Assignee: nobody → kchen
Comment 29•10 years ago
|
||
Any update here? What happened after removing the first SetCSSViewport call?
Flags: needinfo?(kchen)
Assignee | ||
Comment 30•10 years ago
|
||
I found no difference when the first SetCSSViewport is removed. Pushed a try to see if it breaks anything else https://tbpl.mozilla.org/?tree=Try&rev=561293e122fa
Flags: needinfo?(kchen)
Comment 31•10 years ago
|
||
(In reply to <away until June 29> Kan-Ru Chen [:kanru] from comment #21)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #19)
> > And which media queries changed dynamically, and why?
>
> http://hg.mozilla.org/mozilla-central/file/1204667a2935/dom/ipc/TabChild.
> cpp#l738
>
> When TabChild receives before-first-paint, it sets the CSS viewport to a
> default value which triggers one MediumFeaturesChanged. It then sets CSS
> viewport again in HandlePossibleViewportChange which triggers again
> MediumFeaturesChanged.
I think David's asking which of these queries from the stylesheet are getting updated dynamically:
@media (min-width: 768px) and (orientation: portrait)
@media (min-width: 768px) and (orientation: landscape)
@media (min-width: 768px)
The hamachi device tested has a 320 x 480 pixels screen size so none of these rules should ever match in any orientation.
Flags: needinfo?(gduan)
Reporter | ||
Comment 32•10 years ago
|
||
Hi Jet,
those three are for tablet (800 x 1280). We have different UI and UX spec for that device.
Flags: needinfo?(gduan)
Comment 33•10 years ago
|
||
Right. I get that these styles are for a larger device, but:
Device: hamachi, 20140415160202 master-central build.
command: |b2gperf --delay=10 Settings|.
...indicates that the test with perf issues was run on a hamachi, which should not have matched any of those media queries. If they are matching, then we have a different issue. BTW, can you provide perf numbers using "min-device-width" and not "min-width"?
Comment 34•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/settings.js#L119
...says that the Settings app is resized down from 980px, which explains the rule matching. Removing the SetCSSViewport(kDefaultViewportSize) should have fixed that. Can we get timing tests on Kanru's build?
Assignee | ||
Comment 35•10 years ago
|
||
Sorry, I didn't write down the timing from my tests. I remember there is little or no difference before/after I remove SetCSSViewport(kDefaultViewportSize). Maybe it's because I was testing against a DEBUG build.
I'll build a normal build unagi and test it again.
Comment 36•10 years ago
|
||
(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #14)
> Blocking for 1.4 as this is needed by a blocker :
> https://bugzilla.mozilla.org/show_bug.cgi?id=1002842
Looks like 1002842 is alredy fixed, which was the original reason we blocked 1.4 on this. i also spoke offline to :mike and he did not see any other reason we would be blocking this bug for 1.4, So setting this back to nomination 1.4? . Please raise any concerns now for not making this a blocker.
blocking-b2g: 1.4+ → 1.4?
Assignee | ||
Comment 37•10 years ago
|
||
Results for Settings
cold_load_time: median:899, mean:921, std: 60, max:1102, min:872
all:930,1057,913,912,894,895,872,877,880,884,912,1061,900,896,887,
896,878,901,915,1102,915,897,885,901,880,895,916,1063,922,899
Results for Settings w/o SetCSSViewport
cold_load_time: median:761, mean:796, std: 86, max:1164, min:741
all:813,897,773,769,741,760,741,741,758,833,775,862,782,753,743,754,
749,748,756,931,802,764,762,760,760,752,1164,930,783,749
Updated•10 years ago
|
Severity: blocker → normal
Priority: P1 → P2
Comment 38•10 years ago
|
||
Triage: Mike, from comment 36, it looks like we don't need to block on this. Can we unnom? Kan-Ru, from comment 37, it looks like Kan-Ru has found a possible solution as well.
Flags: needinfo?(mlee)
Comment 39•10 years ago
|
||
Per Datazilla (see screenshot), the Settings app's cold load time has been drastically reduced and per @kanru's comment 37 there's no longer a significant difference in cold load time when using SetCSSViewport. Unnoming from 1.4.
Kanru,
Is SetCSSViewport all that needs to be looked at to address the originally reported issue in comment 0? If so, based on your most recent comment please resolve this was WFM.
Flags: needinfo?(mlee) → needinfo?(kchen)
Updated•10 years ago
|
blocking-b2g: 1.4? → ---
Whiteboard: [c=progress p= s= u=1.4] → [c=progress p= s= u=]
Assignee | ||
Comment 40•10 years ago
|
||
(In reply to Mike Lee [:mlee] from comment #39)
> Created attachment 8449667 [details]
> Settings app Screenshot 2014-07-02 at 12.24.51 PM.png
>
> Per Datazilla (see screenshot), the Settings app's cold load time has been
> drastically reduced and per @kanru's comment 37 there's no longer a
> significant difference in cold load time when using SetCSSViewport. Unnoming
> from 1.4.
>
> Kanru,
> Is SetCSSViewport all that needs to be looked at to address the originally
> reported issue in comment 0? If so, based on your most recent comment please
> resolve this was WFM.
Based on the result in comment 37, remove the extra SetCSSViewport could save us 125ms in average. I think it's still worth considering.
We can prove that SetCSSViewport is always called at least once in HandlePossibleViewportChange either via RecvUpdateDimensions or before-first-paint handler so it's safe to remove the extra SetCSSViewport.
Flags: needinfo?(kchen)
Assignee | ||
Comment 41•10 years ago
|
||
Attachment #8449946 -
Flags: review?(bugmail.mozilla)
Comment 42•10 years ago
|
||
Comment on attachment 8449946 [details] [diff] [review]
Remove the extra SetCSSViewport
Review of attachment 8449946 [details] [diff] [review]:
-----------------------------------------------------------------
r=me assuming this has been tested sufficiently. We should keep an eye out for regressions.
Attachment #8449946 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 43•10 years ago
|
||
Comment 44•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
It seems like it is a very important fix for v2.0 from https://bugzilla.mozilla.org/show_bug.cgi?id=1035135#c7
Could you please uplift it to v2.0 ?
Assignee | ||
Comment 46•10 years ago
|
||
Let's wait the patch be cooked a bit longer on trunk. Meanwhile, we could get the measurement to find out if the patch really helps.
Flags: needinfo?(kchen)
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Whiteboard: [c=progress p= s= u=] → [c=progress p= s= u=], [NO_UPLIFT]
Comment 47•10 years ago
|
||
Backed out for causing frequent B2G layout/reftests/svg/sizing reftest failures as seen in bug 1035446, bug 1035443, bug 1034559, bug 1035675, and others.
https://hg.mozilla.org/mozilla-central/rev/0f8a2996abc9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla33 → ---
Assignee | ||
Comment 48•10 years ago
|
||
I can't think of the connection between this patch and the test failures. The failed tests are testing that a width:auto <object> should have the width of the intrinsic width of the wrapped SVG. Unless SetCSSViewport affects that...
Pushed to try.
https://tbpl.mozilla.org/?tree=Try&rev=91a6b8bca5cd
Comment 49•10 years ago
|
||
Backing out this fix caused a 100ms regression in the Settings cold load launch time, as documented in bug 1037542. Granted, the 100ms was a gain that showed up on July 4 from this fix landing the first time. I'll continue to watch this bug, and see what happens.
Comment 50•10 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #48)
> I can't think of the connection between this patch and the test failures.
They certainly went away post-backout. And your Try push shows the same type of failure it was backed out for...
Assignee | ||
Comment 51•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #50)
> (In reply to Kan-Ru Chen [:kanru] from comment #48)
> > I can't think of the connection between this patch and the test failures.
>
> They certainly went away post-backout. And your Try push shows the same type
> of failure it was backed out for...
Right, I also reproduced it locally with emulator..
Updated•10 years ago
|
Severity: normal → blocker
Priority: P2 → P1
Whiteboard: [c=progress p= s= u=], [NO_UPLIFT] → [c=progress p= s= u=2.0], [NO_UPLIFT]
Comment 52•10 years ago
|
||
That test came in with bug 294086. What are the return values for svgElem->GetViewportSize() with and without the patch?
Assignee | ||
Comment 53•10 years ago
|
||
Currently it's very hard to reproduce it locally.. I'll try to get the value.
Comment 54•10 years ago
|
||
The problem of this bug was addressed in bug 1024893. In this case, I suggest to remove 2.0+ and also remove from QC CS list.
blocking-b2g: 2.0+ → 2.0?
Comment 55•10 years ago
|
||
(In reply to Kevin Hu [:khu] from comment #54)
> The problem of this bug was addressed in bug 1024893. In this case, I
> suggest to remove 2.0+ and also remove from QC CS list.
Tapas, any concern with doing this?
Severity: blocker → normal
Flags: needinfo?(tkundu)
Priority: P1 → P2
Whiteboard: [c=progress p= s= u=2.0], [NO_UPLIFT] → [c=progress p= s= u=], [NO_UPLIFT]
(In reply to Mike Lee [:mlee] from comment #55)
> (In reply to Kevin Hu [:khu] from comment #54)
> > The problem of this bug was addressed in bug 1024893. In this case, I
> > suggest to remove 2.0+ and also remove from QC CS list.
>
> Tapas, any concern with doing this?
I am fine with it.
Flags: needinfo?(tkundu)
Comment 57•10 years ago
|
||
2.0- and removed from QC CS list per comment 56.
No longer blocks: CAF-v2.0-FC-metabug
blocking-b2g: 2.0? → ---
Assignee | ||
Comment 58•10 years ago
|
||
I tried to run the tests in emulator. First I disabled reftest caching so I could run multiple times a test. For each invocation of ./mach reftest-remote --enable-oop layout/reftests/svg/sizing/reftest.list TabChild::RecvUpdateDimensions is called once at the beginning and each subsequent document reload will receive before-first-paint.
TabChild::RecvUpdateDimensions
TabChild::HandlePossibleViewportChange()
SetCSSViewport(800, 1000) <--- Does not really call nsIDOMWindowUtils::SetCSSViewport
before-first-paint
SetCSSViewport(980, 480) <--- removed in patch
TabChild::HandlePossibleViewportChange()
SetCSSViewport(800, 1000)
The first call to SetCSSViewport has no effect because mContentDocumentIsDisplayed is false. So the only difference is that without the patch, the CSS viewport is first set to a default size then changed to the real size.
Maybe some code path is only run after view port size change?
Updated•10 years ago
|
Updated•10 years ago
|
Flags: needinfo?(jwatt)
Updated•10 years ago
|
Flags: needinfo?(jwatt)
Comment 60•10 years ago
|
||
For middle term it's pretty important for resolving webapp load time performance with @media tag, or we'll have to do some workaround on build time (we have did that in settings).
Due to load time impact, this issue also make future non-phone layout hardly pass the review to merge into gaia code base.
3rd-party developers will still suffer for this load time penalty with @media.
ni device team, what can we do to get this back on track?
Flags: needinfo?(ehung)
Comment 61•10 years ago
|
||
I think the best way is to nominate it as a blocker of this release.
[Blocking Requested - why for this release]: app performance issue.
blocking-b2g: --- → 2.1?
Flags: needinfo?(ehung)
Updated•10 years ago
|
Flags: needinfo?(jwatt)
Comment 62•10 years ago
|
||
jwatt:
I suspect the patch in this bug makes us not enter the code after this if():
http://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGOuterSVGFrame.cpp#359
It looks like that check may be flawed, since an uninitialized device viewport is {0,0} and matches an uninitialized SVG viewport. Should we indicate that case and force the SetViewport() call if both are uninitialized?
Comment 63•10 years ago
|
||
jet: I don't think that's the problem. As I understand it the patch changes things so that whereas we used to call SetCSSViewport twice - first with arbitrary default dimensions, then with the actual dimensions - now we only make the latter call. Post-patch, when that latter SetCSSViewport call sets the actual (non-zero) dimensions, the code that you point out should be entered since |newViewportSize| will be non-zero sized while |oldViewportSize| will be zero-sized.
Flags: needinfo?(jwatt)
Comment 64•10 years ago
|
||
I think I know what's going on here. It's not that the CSS viewport isn't being set as necessary/correctly, but rather that the improved performance is causing us to occasionally lose in a pre-existing race condition.
First, some observations about the failure mode:
Taking object--auto-auto--px-0.html as an example, this test contains an <object> that should take on the content size 70px x 0px (the size of the embedded SVG document). When this test fails it actually takes on the dimensions 300px x 150px.
This 300x150 size is the fallback size that we use in nsLayoutUtils::ComputeSizeWithIntrinsicDimensions when we can't determine a computed size based on intrinsic dimensions:
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?mark=4323-4323,4331-4331,4352-4352,4367-4367#4315
and also for the intrinsic dimensions of nsSubDocumentFrame:
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?mark=575-575,592-592#560
So it's expected that the <object> will be given computed size 300x150 if it is reflowed before the SVG document that it contains has loaded. Those dimensions should be transient though. Once the SVG document loads we should schedule a reflow of the embedding element if necessary.
As far as I can tell we currently have two mechanisms to trigger a reflow of the embedding element. First, if the 'data' attribute of an <object> changes (loading a new embedded document) we post an nsChangeHint_ReconstructFrame restyle event. Second, we have code to handle reflow of the embedding element as necessary if the intrinsic dimensions of an embedded SVG document change:
https://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGOuterSVGFrame.cpp?mark=674-675#670
We won't hit either of these cases in the SVG 'sizing' tests though, since when the nsSubDocumentFrame gets its first reflow the 'data' attribute will already have been set, and since the width/height attributes of the embedded SVG don't change we won't end up in nsSVGOuterSVGFrame::AttributeChanged either.
So basically we rely on the embedded SVG document already having had an nsSVGOuterSVGFrame by the time that the reflow of the embedding <object> element is reflowed. (We don't actually require that the nsSVGOuterSVGFrame has had a reflow, only that it exists since nsSubDocumentFrame::ComputeSize uses it to read the SVG's intrinsic dimensions.) If that is not the case then the nsSubDocumentFrame will get the 300x150 size and there's no mechanism that will subsequently resize it once the embedded SVG document finishes loading.
Comment 65•10 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #64)
> I think I know what's going on here. It's not that the CSS viewport isn't
> being set as necessary/correctly, but rather that the improved performance
> is causing us to occasionally lose in a pre-existing race condition.
Any thoughts on how to get around the race? If this is pre-existing, can we fix it with test timeouts?
Comment 66•10 years ago
|
||
(In reply to Jet Villegas (:jet) from comment #65)
> Any thoughts on how to get around the race? If this is pre-existing, can we
> fix it with test timeouts?
I'd rather not since it seems users may be more likely to encounter this pre-existing bug once the patch for this bug lands. I'd rather fix the pre-existing bug, for which I've put up a patch for review in bug 1063073.
Comment 67•10 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #66)
> (In reply to Jet Villegas (:jet) from comment #65)
> > Any thoughts on how to get around the race? If this is pre-existing, can we
> > fix it with test timeouts?
>
> I'd rather not since it seems users may be more likely to encounter this
> pre-existing bug once the patch for this bug lands. I'd rather fix the
> pre-existing bug, for which I've put up a patch for review in bug 1063073.
Sweet!
Assignee | ||
Comment 68•10 years ago
|
||
(In reply to Jet Villegas (:jet) from comment #67)
> (In reply to Jonathan Watt [:jwatt] from comment #66)
> > (In reply to Jet Villegas (:jet) from comment #65)
> > > Any thoughts on how to get around the race? If this is pre-existing, can we
> > > fix it with test timeouts?
> >
> > I'd rather not since it seems users may be more likely to encounter this
> > pre-existing bug once the patch for this bug lands. I'd rather fix the
> > pre-existing bug, for which I've put up a patch for review in bug 1063073.
>
> Sweet!
Cool! So this means we could land again!
Comment 69•10 years ago
|
||
Yes. I just repushed for you:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d30e40d7fc0
Updated•10 years ago
|
Assignee: nobody → kchen
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Comment 70•10 years ago
|
||
The patch for bug 1063073 should be safe to backport to branches so if you want to try and get the fix for this bug on branches that would be fine.
Comment 71•10 years ago
|
||
The patch here has also resulted if layout/reftests/svg/svg-integration/filter-html-01-extref.xhtml failing intermittently, which seems like another race. On balance I think it's better to take the patch though and work out the filter-html-01-extref.xhtml failure. I've marked the test as random for B2G and opened bug 1063987 for figuring out and fixing the issue.
Comment 72•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•10 years ago
|
Comment 74•10 years ago
|
||
See comment 49 for the kind of start up time improvement we'd see with this if approved for 2.1.
Comment 75•10 years ago
|
||
Discussed in triage, and since the workaround leveled out the regression, this is pure perf win. We don't block on those, but definitely would definitely take the patch if approved for 2.1 landing by sheriffs (Fabrice, Bhavana).
Ask for approval on the patch :)
(For clarification, the difference between blocking and approval: blocking+ means we don't ship until it's fixed. approval+ on the patch means we'll take it, but if it causes problems we back it out and ship anyway.)
blocking-b2g: 2.1? → -
Comment 76•10 years ago
|
||
I'm away right now. Can someone on the B2G team pick up getting approvals, assuming you guys want that?
Flags: needinfo?(kchen)
Assignee | ||
Comment 77•10 years ago
|
||
Comment on attachment 8449946 [details] [diff] [review]
Remove the extra SetCSSViewport
Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: App/page load is slower because of unnecessary reflow. See comment 49 for the improvements.
[Describe test coverage new/current, TBPL]: Landed on m-c for a while.
[Risks and why]: Little, and the patch is easy to backout.
[String/UUID change made/needed]: NA
This should be uplifted together with bug 1063073 otherwise there will be intermittent test failures.
Attachment #8449946 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(kchen)
Comment 78•10 years ago
|
||
Comment on attachment 8449946 [details] [diff] [review]
Remove the extra SetCSSViewport
Bhavana - We really need a way to track B2G bugs so that bugs like this don't sit for a month and then get very close to missing their chance for uplift.
Aurora+
Attachment #8449946 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bbajaj)
Updated•10 years ago
|
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [c=progress p= s= u=], [NO_UPLIFT] → [c=progress p= s= u=]
Comment 79•10 years ago
|
||
Comment 80•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] from comment #78)
> Comment on attachment 8449946 [details] [diff] [review]
> Remove the extra SetCSSViewport
>
> Bhavana - We really need a way to track B2G bugs so that bugs like this
> don't sit for a month and then get very close to missing their chance for
> uplift.
>
> Aurora+
sure, lets discuss offline. But, in this case it would anyways not be on release management's radar as it was not being tracked/blocked and could have ridden the trains as well.
:kanru, was this a missed uplift ?
Flags: needinfo?(bbajaj)
Assignee | ||
Comment 81•10 years ago
|
||
(In reply to bhavana bajaj [:bajaj] from comment #80)
> (In reply to Lawrence Mandel [:lmandel] from comment #78)
> > Comment on attachment 8449946 [details] [diff] [review]
> > Remove the extra SetCSSViewport
> >
> > Bhavana - We really need a way to track B2G bugs so that bugs like this
> > don't sit for a month and then get very close to missing their chance for
> > uplift.
> >
> > Aurora+
>
> sure, lets discuss offline. But, in this case it would anyways not be on
> release management's radar as it was not being tracked/blocked and could
> have ridden the trains as well.
>
> :kanru, was this a missed uplift ?
See comment 75. I think it slipped because no one actually requested approval until comment 76. How do we mark these "nice to have" bug?
You need to log in
before you can comment on or make changes to this bug.
Description
•