Closed Bug 1088760 Opened 5 years ago Closed 3 years ago

Remove nsRenderingContext

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

I've now completed bug 1087958, so nsRenderingContext is now just an empty wrapper around a gfxContext/DrawTarget that provides no functionality.

Converting the close to 1000 nsRenderingContext instances and arguments to gfxContext at this stage would be a waste of coding/review time though. I've been finding that as I've been converting code to DrawTarget the number of instances of the string "nsRenderingContext" in the code has been dropping rapidly anyway. Rather than doing a huge conversion from nsRenderingContext to gfxContext and then later from gfxContext to DrawTarget, I think we should just wait for the number of nsRenderingContext instances to drop via the Moz2D conversion process and then do a direct nsRenderingContext -> DrawTarget conversion once we get to an appropriate point. That way we save ourselves some work.
Depends on: 651021
Depends on: 1091321
Depends on: 1091323
Depends on: 1231550
Hey jwatt! I was working on refactoring a bunch of code to use DrawTarget and stumbled across the results of your work -- that nsRenderingContext just wraps gfxContext. It looked like the replacement work was trivial, so I wrote up a quick patch with some find-and-replace with manual cleanup. While finishing up the cleanup I came across this bug -- do you still think going forward with this would be a waste of time?
If the patches are fairly straightforward to review I can review what you've done. If they do things like change pointers to references (and require lots of related review work to sanity check) then it may not be the best use of review time. If you can break up the patches into manageable, standalone, compilable chunks (sub 50 KB chunks ideally, but definitely sub 100 KB chucks) I'm happy to take a look. If you've converted everything then attach the patches here, but if there is still outstanding work then probably file one or more blocker bugs (like the existing, fixed blockers) and attach the patches there.
Patch pushed: it's almost entirely completely boring mechanical work, as most uses of nsRenderingContext are just feeding through to other APIs that require nsRenderingContext.

The only real work happens at the boundaries between APIs that take nsRenderingContext and gfxContext. For those it's mostly just eliminating a temporary variable for the conversion. Sometimes this leads to the used variable changing from a reference to a pointer, but the fallout of this is pretty boring (and trivially caught by the compiler if an error is made).
Comment on attachment 8876822 [details]
Bug 1088760 - Remove nsRenderingContext, replacing all of its uses with gfxContext.

https://reviewboard.mozilla.org/r/148140/#review152506

Looks good. We can pick up any fixes in a post commit review. Let's get this in ASAP to avoid having it rot due to churn.
Attachment #8876822 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8876822 [details]
Bug 1088760 - Remove nsRenderingContext, replacing all of its uses with gfxContext.

https://reviewboard.mozilla.org/r/148140/#review152550

Lovely.

::: gfx/src/nsFontMetrics.h:203
(Diff revision 1)
>                       DrawTarget* aDrawTarget);
>  
>      // Draw a string using this font handle on the surface passed in.
>      void DrawString(const char *aString, uint32_t aLength,
>                      nscoord aX, nscoord aY,
> -                    nsRenderingContext *aContext);
> +                    gfxContext *aContext);

Don't fix this now (get this landed, as Jeff says), but for future reference while you're touching lines like this you might as well type-hug the "*" as per our style guidelines.
Attachment #8876822 - Flags: review?(jwatt) → review+
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1ce85e6348
Remove nsRenderingContext, replacing all of its uses with gfxContext. r=jwatt,jrmuizel
Backed out for bustage, at least on Android at layout/generic/nsPluginFrame.cpp:1612:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1940873102d01722956b79991166286e121072a

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3d1ce85e6348307a1e98284e6d13da828729bf91&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Bustage log: https://treeherder.mozilla.org/logviewer.html#?job_id=106479413&repo=mozilla-inbound

[task 2017-06-12T22:24:35.079183Z] 22:24:35     INFO -  /home/worker/workspace/build/src/layout/generic/nsPluginFrame.cpp: In member function 'void nsPluginFrame::PaintPlugin(nsDisplayListBuilder*, gfxContext&, const nsRect&, const nsRect&)':
[task 2017-06-12T22:24:35.079294Z] 22:24:35     INFO -  /home/worker/workspace/build/src/layout/generic/nsPluginFrame.cpp:1612:72: error: no matching function for call to 'nsPluginInstanceOwner::Paint(gfxContext&, gfxRect&, gfxRect&)'
[task 2017-06-12T22:24:35.079338Z] 22:24:35     INFO -       mInstanceOwner->Paint(aRenderingContext, frameGfxRect, dirtyGfxRect);
[task 2017-06-12T22:24:35.079375Z] 22:24:35     INFO -                                                                          ^
[task 2017-06-12T22:24:35.080718Z] 22:24:35     INFO -  /home/worker/workspace/build/src/layout/generic/nsPluginFrame.cpp:1612:72: note: candidate is:
[task 2017-06-12T22:24:35.081411Z] 22:24:35     INFO -  In file included from /home/worker/workspace/build/src/layout/generic/nsPluginFrame.cpp:50:0:
[task 2017-06-12T22:24:35.082103Z] 22:24:35     INFO -  /home/worker/workspace/build/src/dom/plugins/base/nsPluginInstanceOwner.h:112:8: note: void nsPluginInstanceOwner::Paint(gfxContext*, const gfxRect&, const gfxRect&)
[task 2017-06-12T22:24:35.082969Z] 22:24:35     INFO -     void Paint(gfxContext* aContext,
[task 2017-06-12T22:24:35.083776Z] 22:24:35     INFO -          ^
[task 2017-06-12T22:24:35.084641Z] 22:24:35     INFO -  /home/worker/workspace/build/src/dom/plugins/base/nsPluginInstanceOwner.h:112:8: note:   no known conversion for argument 1 from 'gfxContext' to 'gfxContext*'
[task 2017-06-12T22:24:35.085469Z] 22:24:35     INFO -  /home/worker/workspace/build/src/config/rules.mk:1064: recipe for target 'nsPluginFrame.o' failed
[task 2017-06-12T22:24:35.086294Z] 22:24:35     INFO -  gmake[5]: *** [nsPluginFrame.o] Error 1

(In reply to Nicholas Nethercote [:njn] from comment #8)
> yay!
http://i.imgur.com/jLUAQuo.gif
Flags: needinfo?(jmuizelaar)
Ah, I only checked compilation on linux/mac. I'll kick off a try on all platforms to check all the ifdefs.
Oh yes, an all-platforms try build is definitely a good idea before pushing any large refactoring change :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bb830220893e021ca9b04570e8f7e2f2e18e727&selectedJob=106520865

Seems good. Only change was fixing that one line that Sebastien posted. Pushed to mozreview.
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5dda793c3e
Remove nsRenderingContext, replacing all of its uses with gfxContext. r=jwatt,jrmuizel
https://hg.mozilla.org/mozilla-central/rev/5e5dda793c3e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.