Closed Bug 919129 Opened 7 years ago Closed 7 years ago

fix nsLayoutUtils::UpdateImageVisibilityForFrame to not expand the image rect too much

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
nsLayoutUtils::UpdateImageVisibilityForFrame expands the image rect to the scrollport of the scrollframe for each scroll frame it encounters. This is way too much.

The rect of the image may be scrolled out of view in the scroll frame. Using a rect that is scrolled out of view for further visibility calculations seems wrong. Instead pretend the rect was scrolled into view.
Attachment #808101 - Flags: review?(matspal)
This makes sense for a small image (relative the scroll port), but it seems overly pessimistic
for an image that's larger than the scroll port.  I think we want to clip the result to the
scroll port after the lines you added.  (which, given that x,y is now inside the scroll port
with the lines you added, boils down to std::max of the two widths/heights)

Also,
  transformedRect.x += scrollPort.x - transformedRect.x;
is the same as:
  transformedRect.x = scrollPort.x;
Er, std::min that is... bah, reviewing while having morning coffee ;-)
(In reply to Mats Palmgren (:mats) from comment #1)
> I think we want to clip the result to the
> scroll port after the lines you added.

Yes of course. This was what I always intended to do but forget to add after I had the shifting part done.
Attached patch patch v2Splinter Review
Attachment #808101 - Attachment is obsolete: true
Attachment #808101 - Flags: review?(matspal)
Attachment #808202 - Flags: review?(matspal)
Comment on attachment 808202 [details] [diff] [review]
patch v2

OK, looks good.  r=mats
Attachment #808202 - Flags: review?(matspal) → review+
Bug 847223 added this code, so adding dependency to track that.
Blocks: 847223
https://hg.mozilla.org/mozilla-central/rev/9bd43aa9a018
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 808202 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 847223
User impact if declined: this patch just fixes the code that was added in bug 847223 to determine if a image is visible or not. it stops images from being considered visible or nearly visible when they are not. taking this patch means we only have one version of this code around and never ship the sub-optimal code.
Testing completed (on m-c, etc.): been on m-c for a while now, no problems found
Risk to taking this patch (and alternatives if risky): not risky
String or IDL/UUID changes made by this patch: none
Attachment #808202 - Flags: approval-mozilla-aurora?
blocking-b2g: --- → koi?
Attachment #808202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We probably don't need to bother triaging this for koi at this point, given that this is already landed on aurora per comment 10.
blocking-b2g: koi? → ---
Whiteboard: [qa-]
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.