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)
Core
CSS Parsing and Computation
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.
Assignee | ||
Comment 1•8 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
Comment 4•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
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
Comment 8•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•