Closed Bug 1349849 Opened 6 years ago Closed 6 years ago

remove WidgetStateChanged() call from nsScrollbarFrame::MoveToNewPosition()

Categories

(Core :: XUL, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: karlt, Assigned: karlt)

Details

Attachments

(2 files)

No description provided.
Comment on attachment 8850354 [details]
bug 1349849 document nsITheme::WidgetStateChanged()

https://reviewboard.mozilla.org/r/122970/#review125470
Attachment #8850354 - Flags: review?(mstange) → review+
Comment on attachment 8850355 [details]
bug 1349849 remove WidgetStateChanged() call from nsScrollbarFrame::MoveToNewPosition()

https://reviewboard.mozilla.org/r/122972/#review125474

Looks good, assuming as we still invalidate the scrollbar buttons on Linux in the right cases because of some other notification.
Attachment #8850355 - Flags: review?(mstange) → review+
Comment on attachment 8850355 [details]
bug 1349849 remove WidgetStateChanged() call from nsScrollbarFrame::MoveToNewPosition()

https://reviewboard.mozilla.org/r/122972/#review125474

Thanks.  I'm not clear what you mean by "still".  This patch doesn't change
any invalidation.

Changes in
https://hg.mozilla.org/mozilla-central/rev/9eabf947efc3363a1bf79aa03c3053d184510846
may have changed invalidation, but I don't know what all the right cases are.

I don't know what actually depends on the attribute changes in
nsScrollbarFrame::MoveToNewPosition().  Scrollbar button presses, which use
MoveToNewPosition(), use async scrolling.  And in my experimentation, even
synchronous scrolls go via ScrollFrameHelper::CompleteAsyncScroll(), which
sets curpos with notifications.

#0  0x00007fd0fb8d1ec6 in nsNativeThemeGTK::WidgetStateChanged(nsIFrame*, uint8_t, nsIAtom*, bool*, nsAttrValue const*) (this=0x7fd06227a800, aFrame=0x7fd0b7fdc868, aWidgetType=84 'T', aAttribute=0x7fd0dd3aeac0, aShouldRepaint=0x7ffc9843e740, aOldValue=0x7ffc9843ec50)
    at /mnt/ssd1/karl/moz/dev/widget/gtk/nsNativeThemeGTK.cpp:1747
#1  0x00007fd0fbc36f09 in (anonymous namespace)::GeckoRestyleManager::AttributeChanged((anonymous namespace)::RestyleManager::Element*, int32_t, nsIAtom*, int32_t, nsAttrValue const*) (this=0x7fd09a1a1480, aElement=0x7fd0623ffc10, aNameSpaceID=0, aAttribute=0x7fd0dd3aeac0, aModType=1, aOldValue=0x7ffc9843ec50)
    at /mnt/ssd1/karl/moz/dev/layout/base/GeckoRestyleManager.cpp:308
#2  0x00007fd0fbc80ee7 in (anonymous namespace)::RestyleManager::AttributeChanged((anonymous namespace)::dom::Element*, int32_t, nsIAtom*, int32_t, nsAttrValue const*) (this=0x7fd09a1a1480, aElement=0x7fd0623ffc10, aNameSpaceID=0, aAttribute=0x7fd0dd3aeac0, aModType=1, aOldValue=0x7ffc9843ec50)
    at /mnt/sda11/karl/obj/dist/include/mozilla/RestyleManagerInlines.h:72
#3  0x00007fd0fbc564ff in (anonymous namespace)::PresShell::AttributeChanged(nsIDocument*, (anonymous namespace)::dom::Element*, int32_t, nsIAtom*, int32_t, nsAttrValue const*) (this=0x7fd063fad800, aDocument=0x7fd063ef0000, aElement=0x7fd0623ffc10, aNameSpaceID=0, aAttribute=0x7fd0dd3aeac0, aModType=1, aOldValue=0x7ffc9843ec50) at /mnt/ssd1/karl/moz/dev/layout/base/PresShell.cpp:4373
#4  0x00007fd0f99d06a1 in nsNodeUtils::AttributeChanged((anonymous namespace)::dom::Element*, int32_t, nsIAtom*, int32_t, nsAttrValue const*) (aElement=0x7fd0623ffc10, aNameSpaceID=0, aAttribute=0x7fd0dd3aeac0, aModType=1, aOldValue=0x7ffc9843ec50) at /mnt/ssd1/karl/moz/dev/dom/base/nsNodeUtils.cpp:145
#5  0x00007fd0f97f5ff9 in (anonymous namespace)::dom::Element::SetAttrAndNotify(int32_t, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, uint8_t, bool, bool, bool) (this=0x7fd0623ffc10, aNamespaceID=0, aName=0x7fd0dd3aeac0, aPrefix=0x0, aOldValue=..., aParsedValue=..., aModType=1 '\001', aFireMutation=false, aNotify=true, aCallAfterSetAttr=true)
    at /mnt/ssd1/karl/moz/dev/dom/base/Element.cpp:2501
#6  0x00007fd0f97f54fa in (anonymous namespace)::dom::Element::SetAttr(int32_t, nsIAtom*, nsIAtom*, nsAString const&, bool) (this=0x7fd0623ffc10, aNamespaceID=0, aName=0x7fd0dd3aeac0, aPrefix=0x0, aValue=..., aNotify=true)
    at /mnt/ssd1/karl/moz/dev/dom/base/Element.cpp:2364
#7  0x00007fd0f90c0fa2 in nsIContent::SetAttr(int32_t, nsIAtom*, nsAString const&, bool) (this=0x7fd0623ffc10, aNameSpaceID=0, aName=0x7fd0dd3aeac0, aValue=..., aNotify=true) at /mnt/ssd1/karl/moz/dev/dom/base/nsIContent.h:364
#8  0x00007fd0fb5f6e40 in nsXBLPrototypeBinding::AttributeChanged(nsIAtom*, int32_t, bool, nsIContent*, nsIContent*, bool) (this=0x7fd0623c8a80, aAttribute=0x7fd0dd3aeac0, aNameSpaceID=0, aRemoveFlag=false, aChangedElement=0x7fd0623ff5e0, aAnonymousContent=0x7fd0623ffb80, aNotify=true)
    at /mnt/ssd1/karl/moz/dev/dom/xbl/nsXBLPrototypeBinding.cpp:379
#9  0x00007fd0fb5e419b in nsXBLBinding::AttributeChanged(nsIAtom*, int32_t, bool, bool) (this=0x7fd0b813bba0, aAttribute=0x7fd0dd3aeac0, aNameSpaceID=0, aRemoveFlag=false, aNotify=true)
    at /mnt/ssd1/karl/moz/dev/dom/xbl/nsXBLBinding.cpp:615
#10 0x00007fd0f97f5c23 in (anonymous namespace)::dom::Element::SetAttrAndNotify(int32_t, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, uint8_t, bool, bool, bool) (this=0x7fd0623ff5e0, aNamespaceID=0, aName=0x7fd0dd3aeac0, aPrefix=0x0, aOldValue=..., aParsedValue=..., aModType=1 '\001', aFireMutation=false, aNotify=true, aCallAfterSetAttr=true)
    at /mnt/ssd1/karl/moz/dev/dom/base/Element.cpp:2465
#11 0x00007fd0f97f54fa in (anonymous namespace)::dom::Element::SetAttr(int32_t, nsIAtom*, nsIAtom*, nsAString const&, bool) (this=0x7fd0623ff5e0, aNamespaceID=0, aName=0x7fd0dd3aeac0, aPrefix=0x0, aValue=..., aNotify=true)
    at /mnt/ssd1/karl/moz/dev/dom/base/Element.cpp:2364
#12 0x00007fd0f90c0fa2 in nsIContent::SetAttr(int32_t, nsIAtom*, nsAString const&, bool) (this=0x7fd0623ff5e0, aNameSpaceID=0, aName=0x7fd0dd3aeac0, aValue=..., aNotify=true) at /mnt/ssd1/karl/moz/dev/dom/base/nsIContent.h:364
#13 0x00007fd0fbe05eaa in (anonymous namespace)::ScrollFrameHelper::SetCoordAttribute(nsIContent*, nsIAtom*, nscoord) (this=0x7fd063fcc538, aContent=0x7fd0623ff5e0, aAtom=0x7fd0dd3aeac0, aSize=600)
    at /mnt/ssd1/karl/moz/dev/layout/generic/nsGfxScrollFrame.cpp:5683
#14 0x00007fd0fbe01d59 in (anonymous namespace)::ScrollFrameHelper::UpdateScrollbarPosition() (this=0x7fd063fcc538)
    at /mnt/ssd1/karl/moz/dev/layout/generic/nsGfxScrollFrame.cpp:4614
#15 0x00007fd0fbdfb57d in (anonymous namespace)::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, nsIAtom*) (this=0x7fd063fcc538, aPt=..., aRange=..., aOrigin=0x7fd0dd3b96d0)
    at /mnt/ssd1/karl/moz/dev/layout/generic/nsGfxScrollFrame.cpp:2910
#16 0x00007fd0fbdf9105 in (anonymous namespace)::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, nsIAtom*) (this=0x7fd063fcc538, aRange=..., aOrigin=0x7fd0dd3b96d0)
    at /mnt/ssd1/karl/moz/dev/layout/generic/nsGfxScrollFrame.cpp:2155
#17 0x00007fd0fbdf96a7 in (anonymous namespace)::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, nsIScrollableFrame::ScrollMode, nsIAtom*, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) (this=0x7fd063fcc538, aScrollPosition=..., aMode=nsIScrollableFrame::INSTANT, aOrigin=0x7fd0dd3b96d0, aRange=0x7ffc9843f8f0, aSnap=nsIScrollbarMediator::DISABLE_SNAP)
    at /mnt/ssd1/karl/moz/dev/layout/generic/nsGfxScrollFrame.cpp:2273
#18 0x00007fd0fbe3f0d2 in (anonymous namespace)::ScrollFrameHelper::ScrollTo(nsPoint, nsIScrollableFrame::ScrollMode, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) (this=0x7fd063fcc538, aScrollPosition=..., aMode=nsIScrollableFrame::INSTANT, aRange=0x7ffc9843f8f0, aSnap=nsIScrollbarMediator::DISABLE_SNAP)
    at /mnt/ssd1/karl/moz/dev/layout/generic/nsGfxScrollFrame.h:226
#19 0x00007fd0fbdf935b in (anonymous namespace)::ScrollFrameHelper::ScrollToCSSPixels((anonymous namespace)::ScrollFrameHelper::CSSIntPoint const&, nsIScrollableFrame::ScrollMode) (this=0x7fd063fcc538, aScrollPosition=..., aMode=nsIScrollableFrame::INSTANT)
    at /mnt/ssd1/karl/moz/dev/layout/generic/nsGfxScrollFrame.cpp:2215
#20 0x00007fd0fbe3fcc2 in nsHTMLScrollFrame::ScrollToCSSPixels(nsHTMLScrollFrame::CSSIntPoint const&, nsIScrollableFrame::ScrollMode) (this=0x7fd063fcc4b0, aScrollPosition=..., aMode=nsIScrollableFrame::INSTANT)
    at /mnt/ssd1/karl/moz/dev/layout/generic/nsGfxScrollFrame.h:835
#21 0x00007fd0f975a071 in nsGlobalWindow::ScrollTo((anonymous namespace)::CSSIntPoint const&, (anonymous namespace)::dom::ScrollOptions const&) (this=0x7fd063e46000, aScroll=..., aOptions=...)
    at /mnt/ssd1/karl/moz/dev/dom/base/nsGlobalWindow.cpp:8403

Button attributes are inherited from scrollbar through XBL:

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/toolkit/content/widgets/scrollbar.xml#25
(In reply to Karl Tomlinson (:karlt) from comment #5)
> Comment on attachment 8850355 [details]
> bug 1349849 remove WidgetStateChanged() call from
> nsScrollbarFrame::MoveToNewPosition()
> 
> https://reviewboard.mozilla.org/r/122972/#review125474
> 
> Thanks.  I'm not clear what you mean by "still".  This patch doesn't change
> any invalidation.

Yes, sorry, I realized that after I had submitted the review - if there's a missing invalidation, then it would already have been missing, and not due to your patch.

I'll look at the rest of your comment next week. Feel free to land in the meantime.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7729216c6550 -d e05359a30577: rebasing 385754:7729216c6550 "bug 1349849 document nsITheme::WidgetStateChanged() r=mstange"
rebasing 385755:41a334d01ee4 "bug 1349849 remove WidgetStateChanged() call from nsScrollbarFrame::MoveToNewPosition() r=mstange" (tip)
merging layout/xul/nsScrollbarFrame.cpp
warning: conflicts while merging layout/xul/nsScrollbarFrame.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7aa9d5c0a026
document nsITheme::WidgetStateChanged() r=mstange
https://hg.mozilla.org/integration/autoland/rev/05814c43dab7
remove WidgetStateChanged() call from nsScrollbarFrame::MoveToNewPosition() r=mstange
https://hg.mozilla.org/mozilla-central/rev/7aa9d5c0a026
https://hg.mozilla.org/mozilla-central/rev/05814c43dab7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.