Closed Bug 895003 Opened 11 years ago Closed 11 years ago

Need paint flashing *only* for chrome

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: bugs, Assigned: milan)

References

Details

Attachments

(1 file, 1 obsolete file)

I think we need the inverse of bug 847890 so our front-end hackers can diagnose invalidation problems without getting psychedelic seizures as they surf the web.
Looks like we need a check for PresContext()->IsChrome() and a bool pref in there somewhere.
There are a few details to sort out. I'm including Rob, Boris and Paul as well because they were involved in bug 847890.  Sorry for the needinfo, if you don't have an opinion or time, just clear it.

Today we have a preference (nglayout.debug.paint_flashing) that turns the flashing on everywhere. In the WebDeveloper toolbox, we have a way of overriding that preference for one tab/context only. The default behaviour is based on the preference, the toolbox button changes it. If you reload the page, you go back to the default/preference.

Few obvious design choices:

We could add another preference (nglayout.debug.paint_flashing_chrome). Chrome would then flash only when this preference is set (Plan A), or when either this, or the original preference is set (Plan B.)

We could start using the current preference (nglayout.debug.paint_flashing) only for chrome (Plan C). The toolbox button would still work for tabs/content. This will look like a regression to users that expect to set that preference and see everything flashing.  It does require an extra click after each reload, and it requires you to open the toolbox if you want to see this.  

I like the simplicity of C, but needing to have the toolbox up to enable this may be too much.  B, on the other hand, is the only one without a "regression" behaviour, though I imagine that there wouldn't be a lot of noise there.

Thoughts on A, B or C?  Or D - another definition?
Flags: needinfo?(roc)
Flags: needinfo?(paul)
Flags: needinfo?(bzbarsky)
Also, is it acceptable that the chrome would be a "startup only" preference, however it is set?
Either A or B sounds fine to me, I think.

It also seems like there's no reason it couldn't be a live preference...
Flags: needinfo?(bzbarsky)
Just got a use case where C means we can't test something, so C is out of the running.
A suggestion as to who should review the result would help.
Comment on attachment 778058 [details] [diff] [review]
Plan B implementation (see comment 2).

Introduce a second preference to control the chrome paint flashing - nglayout.debug.paint_flashing_chrome. The original pref still controls content and chrome when set. The preference is now live, for content and chrome, the change will be seen on the next refresh. The cached value is now in nsPresContext, rather than nsRefreshDriver.
A or B sounds fine to me.
Flags: needinfo?(roc)
Today, you can already start paint flashing only for chrome:

- start the command line (shift-F2)
- type: "paintflashing on --chrome"

If you want to do that from JS:

> window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).paintFlashing = true
Flags: needinfo?(paul)
(In reply to Paul Rouget [:paul] from comment #10)
> Today, you can already start paint flashing only for chrome:
> 
> - start the command line (shift-F2)
> - type: "paintflashing on --chrome"
> 
> If you want to do that from JS:
> 
> > window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).paintFlashing = true

Jet, is this enough, should we close it as "works for me"?  It isn't a preference, but it only has to be done once.
I have the code for the preference, good for review, but if what we have is good enough, we can just make sure people know about it.
Flags: needinfo?(bugs)
My take on my question to Jet - we should have a preference as well.  I imagine some people will run with that on, "all the time" (we do with paint flashing), and having to type it in on every restart would be annoying. I also don't know if there is any useful information in the "what's painted before I had a chance to open the command line and type stuff in" for the chrome, but that was why we killed option C above - we want that for content - it could be the same for chrome.
Rob, you reviewed the original, I'll set the review for this on you, with the assumption that you'll pass it on to somebody else if you don't have time.  I'll leave BenWa in there for review as well, as he seems to be using paint flashing a lot.
Attachment #778058 - Flags: review?(roc)
Attachment #778058 - Flags: review?(bgirard)
Attachment #778058 - Flags: feedback?(bgirard)
My take on this is that we should have the pref as well. If I was a frontend developer I would want to run with this pretty much all the time when I'm developing, and requiring enabling every time you restart means people will forget, and eventually not do it at all.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #13)
> My take on this is that we should have the pref as well. If I was a frontend
> developer I would want to run with this pretty much all the time when I'm
> developing, and requiring enabling every time you restart means people will
> forget, and eventually not do it at all.

+1 here, FWIW.
Flags: needinfo?(bugs)
Bug 895003: Introduce a second preference to control the chrome paint flashing - nglayout.debug.paint_flashing_chrome. The original pref still controls content and chrome when set. The preference is now live, for content and chrome, the change will be seen on the next refresh. The cached value is now in nsPresContext, rather than nsRefreshDriver. r=roc

Carry over r+ from roc - changed the order of initialization in the constructor to account for -Werror=reorder
Attachment #778058 - Attachment is obsolete: true
Attachment #778058 - Flags: review?(bgirard)
Attachment #779330 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d2e122616131
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(This is already finding interesting things - bug 900039 - thanks again)
Blocks: 900422
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: