Bug 36853 - REGRESSION(r56552): Broken scrollbars size
Summary: REGRESSION(r56552): Broken scrollbars size
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Major
Assignee: Antonio Gomes
URL:
Keywords: Qt
Depends on:
Blocks: 35784 36058
  Show dependency treegraph
 
Reported: 2010-03-30 14:22 PDT by Kenneth Rohde Christiansen
Modified: 2010-04-20 13:07 PDT (History)
5 users (show)

See Also:


Attachments
Screenshot 2 (2.22 KB, image/png)
2010-03-30 14:23 PDT, Kenneth Rohde Christiansen
no flags Details
Screenshot 1 (2.02 KB, image/png)
2010-03-30 14:23 PDT, Kenneth Rohde Christiansen
no flags Details
patch v1 (2.85 KB, patch)
2010-04-09 14:50 PDT, Antonio Gomes
kenneth: review-
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
(committed with r57656) patch v2 (2.74 KB, patch)
2010-04-11 19:53 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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