Bug 36853

Summary: REGRESSION(r56552): Broken scrollbars size
Product: WebKit Reporter: Kenneth Rohde Christiansen <kenneth>
Component: New BugsAssignee: Antonio Gomes <tonikitoo>
Status: CLOSED FIXED    
Severity: Major CC: hausmann, hyatt, ossy, tonikitoo, vestbo
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 35784, 36058    
Attachments:
Description Flags
Screenshot 2
none
Screenshot 1
none
patch v1
kenneth: review-, tonikitoo: commit-queue-
(committed with r57656) patch v2 none

Description Kenneth Rohde Christiansen 2010-03-30 14:22:16 PDT
When there is only scroll in one direction the scroll arrows are not at the right position
Comment 1 Kenneth Rohde Christiansen 2010-03-30 14:23:03 PDT
Created attachment 52083 [details]
Screenshot 2
Comment 2 Kenneth Rohde Christiansen 2010-03-30 14:23:21 PDT
Created attachment 52084 [details]
Screenshot 1
Comment 3 Csaba Osztrogonác 2010-04-06 04:13:57 PDT
Expected file and pixel result should be updated after fix in https://bugs.webkit.org/show_bug.cgi?id=36058
Comment 4 Antonio Gomes 2010-04-09 12:11:35 PDT
regression from bug 21300, r56552
Comment 5 Antonio Gomes 2010-04-09 14:50:24 PDT
Created attachment 52991 [details]
patch v1

attempt to get feedback
Comment 6 Kenneth Rohde Christiansen 2010-04-09 14:53:30 PDT
(In reply to comment #5)
> Created an attachment (id=52991) [details]
> patch v1
> 
> attempt to get feedback

What do you mean with attempt? Are you not sure it is fixing the issue? What is the status of this patch?
Comment 7 Kenneth Rohde Christiansen 2010-04-09 14:58:50 PDT
Comment on attachment 52991 [details]
patch v1


> +    int scollbarThickness =
> +#if defined(Q_WS_MAC) && !defined(QT_MAC_USE_COCOA)
> +        ScrollbarTheme::nativeTheme()->scrollbarThickness();
> +#else
> +        0;
> +#endif

I think you should include the 'int scollbarThickness =' inside the ifdef's

The issue that bug 21300 (the regression origin) was working around was a Mac Carbon
 9         fix for the "Grow Box" at the bottom right side of the application window: it was using
 10         a rect sizing scrollbar thickness as the dimensions of this "Grow Box" area.

I would say:

The regression was caused by r56552, which introduced a fix to bug webkit.org/b/21300. 
The bug solved an issue with the resize handle on mac, but did it in a way that 
affected all Qt platforms and thus broke the behavior on non-mac platforms.

This patch makes the mac specific change ifdef'ed and only applied for the mac platform.
Comment 8 Antonio Gomes 2010-04-11 19:49:17 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=52991) [details] [details]
> > patch v1
> > 
> > attempt to get feedback
> 
> What do you mean with attempt? Are you not sure it is fixing the issue? What is
> the status of this patch?

The intention was exactly what you got me in comment #7. thank you, I've changed the ChangeLog content for the better now.
Comment 9 Antonio Gomes 2010-04-11 19:53:36 PDT
Created attachment 53137 [details]
(committed with r57656) patch v2

Differently from patch 52991 (v1), patch v2 makes the whole method body mac cocoa specific. For other platforms it just returns an empty rect as before (similarly on gtk, win, wx, efl and haiku ports).

Thank you for the feedback Kenneth.

Tor Arne, could you also validate that?
Comment 10 Antonio Gomes 2010-04-13 05:47:41 PDT
Ping review! :)
Comment 11 Kenneth Rohde Christiansen 2010-04-14 13:29:05 PDT
Comment on attachment 53137 [details]
(committed with r57656) patch v2

LGTM, r=me.
Comment 12 Tor Arne Vestbø 2010-04-15 10:55:59 PDT
Comment on attachment 53137 [details]
(committed with r57656) patch v2


> +#if defined(Q_WS_MAC) && !defined(QT_MAC_USE_COCOA)

This is not carbon-specific, you should remove the last part of the ifdef.
Comment 13 Antonio Gomes 2010-04-15 11:25:17 PDT
Comment on attachment 53137 [details]
(committed with r57656) patch v2

Clearing flags on attachment: 53137

Committed r57656: <http://trac.webkit.org/changeset/57656>
Comment 14 Antonio Gomes 2010-04-15 11:27:03 PDT
(In reply to comment #12)
> (From update of attachment 53137 [details])
> 
> > +#if defined(Q_WS_MAC) && !defined(QT_MAC_USE_COCOA)
> 
> This is not carbon-specific, you should remove the last part of the ifdef.

Fixed before committing!

Thank you Tor Arne and Kenneth.
Comment 15 Simon Hausmann 2010-04-20 13:07:21 PDT
Revision r57656 cherry-picked into qtwebkit-2.0 with commit c960094253dade2b29e368bf16e48bc9a61da6f6