Closed Bug 1305376 Opened 8 years ago Closed 8 years ago

use nsTArray to store cursor property values

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file)

We currently use a manually allocated array, which is a bit messy. Also we should use nsRuleNode's SetImageRequest function rather than grabbing requests off the css::URLValue.
(In theory we don't need to use SetImageRequest for cursor, since we don't need to do the "make a static request if we're printing" thing for cursor images, which won't be used in a print document, but it will give us a single place to update the logic for getting an imgRequestProxy.)
Comment on attachment 8795098 [details] Bug 1305376 - Use nsTArray for cursor image list storage. https://reviewboard.mozilla.org/r/81260/#review79874 Looks good. r=me with some nits below addressed. ::: layout/style/nsRuleNode.cpp (Diff revision 1) > - delete [] ui->mCursorArray; > - ui->mCursorArray = nullptr; > - ui->mCursorArrayLength = 0; > - I think it is better to put mCursorImages.Clear() here, so that we wouldn't forget to clear it in any branch. ::: layout/style/nsRuleNode.cpp:5095 (Diff revision 1) > "unrecognized cursor unit"); > const nsCSSValueList* list = cursorValue->GetListValue(); > - const nsCSSValueList* list2 = list; > - nsIDocument* doc = aContext->PresContext()->Document(); > - uint32_t arrayLength = 0; > - for ( ; list->mValue.GetUnit() == eCSSUnit_Array; list = list->mNext) > + ui->mCursorImages.Clear(); > + for ( ; list->mValue.GetUnit() == eCSSUnit_Array; list = list->mNext) { > + nsCSSValue::Array* arr = list->mValue.GetArrayValue(); > + SetImageRequest([&](imgRequestProxy* req) { It doesn't seem to me we need SetImageRequest here. Cursor isn't part of rendering, so it is not necessary to use static request. (Actually I doubt we ever compute StyleUserInteface for static document.) ::: layout/style/nsStyleStruct.cpp:3901 (Diff revision 1) > + return mHaveHotspot == aOther.mHaveHotspot && > + mHotspotX == aOther.mHotspotX && > + mHotspotY == aOther.mHotspotY && Probably consider adding a comment or an assertion that when mHaveHotspot is false, mHotsportX and mHotsportY would always be zero, so we don't need to handle case without hotspot specially. (Probably it isn't worth that at all...)
Attachment #8795098 - Flags: review?(xidorn+moz) → review+
In the end I factored out the GetImageValue() call so that for cursor we don't need to do the static request thing.
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e4e13b10667c Use nsTArray for cursor image list storage. r=xidorn
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: