1273013 - download progress bar does not show in gtk3 build.

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Hussam Al-Tayeb]

Attachment: no-progress-bar.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160515172224

Steps to reproduce:

The download progress bar is missing from my gtk3 builds.

Comment 1

[Karl Tomlinson (:karlt)]

What is the GTK version and theme?

Comment 2

[Hussam Al-Tayeb]

Gtk+ 3.20.4 and the default Adwaita theme.
I am pretty sure this was working under gtk+ 2.24 so perhaps something missing in the gtk3 themeing implementation. I will try compiling a gtk2 version tomorrow to verify.

Comment 3

[Karl Tomlinson (:karlt)]

No need to compile with gtk2, thanks.
This was a deliberate change in GTK 3.20.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: wip patch

Needs Bug 1271579 checked in.

Comment 5

[Hussam Al-Tayeb]

Applying both patches to my local tree makes the progress bar appear again. Thank you Martin.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

Managed to test on both 3.20 and 3.16 so it's ready for review. I also changed labels in CreateWidget() to alphabetical order.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8753369 [details] [diff] [review]
patch

Mostly good, thanks.

>+    case MOZ_GTK_PROGRESS_CHUNK:
>+    case MOZ_GTK_PROGRESS_CHUNK_INDETERMINATE:  
>+    case MOZ_GTK_PROGRESS_CHUNK_VERTICAL_INDETERMINATE:
>+      /* Actuall progress bar indicator */
>+      return GetChildNodeStyle(aNodeType,
>+                               MOZ_GTK_PROGRESSBAR,
>+                               "progress",
>+                               MOZ_GTK_PROGRESSBAR_TROUGH);

Because these are all the same, it would be better to cache only one copy of
the style context.  Implement this just for MOZ_GTK_PROGRESS_CHUNK and see
below.

"Actuall" -> "Actual"

The "progress" class is not right for pre-3.20.
See below for some options.

> moz_gtk_progress_chunk_paint(cairo_t *cr, GdkRectangle* rect,
>                              GtkTextDirection direction,
>                              WidgetNodeType widget)
> {

>+    GtkStyleContext* style = ClaimStyleContext(widget, direction);
>+
>+    if (gtk_check_version(3, 20, 0) != nullptr) {
>+        gtk_style_context_remove_class(style, GTK_STYLE_CLASS_TROUGH);
>+        gtk_style_context_add_class(style, GTK_STYLE_CLASS_PROGRESSBAR);
>+    }

For 3.20, pass MOZ_GTK_PROGRESS_CHUNK, instead of |widget| to
ClaimStyleContext().

For pre-3.20, passing MOZ_GTK_PROGRESS_TROUGH is probably the simplest way get
the correct style context.

  If doing that, then please add a comment in GetStyleInternal() that the
  MOZ_GTK_PROGRESS_CHUNK implementation is not used with GTK versions before
  3.20.

  If that seems too weird, then perhaps implement a variant of
  GetChildNodeStyle() for MOZ_GTK_PROGRESS_CHUNK that has both
  aStyleClass and aNodeName parameters.  The four argument version can call
  the five argument version to do the work.
  Passing GTK_STYLE_CLASS_PROGRESSBAR for aStyleClass would mean that the
  add_class() in moz_gtk_progress_chunk_paint() would not be necessary.

  Or refactor the existing GetChildNodeStyle() to call one of two separate
  helper functions depending on GTK version.  From GetStyleInternal(), call a
  special GetProgressChunkStyle() function that uses these helpers and removes
  GTK_STYLE_CLASS_TROUGH when pre-3.20.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.2

Thanks, there's the updated one. 

It's a bit confusing that we need to get MOZ_GTK_PROGRESS_TROUGH instead of MOZ_GTK_PROGRESSBAR just because of the style save/restore in moz_gtk_progress_chunk_paint(). We can consider some mechanism how to explicitly ask ClaimStyleContext() to save/restore the context if we have more such cases.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8754009 [details] [diff] [review]
patch v.2

>+    case MOZ_GTK_PROGRESS_CHUNK_INDETERMINATE:
>+    case MOZ_GTK_PROGRESS_CHUNK_VERTICAL_INDETERMINATE:

If you are happy to remove these two now-unused case-labels, it would make it clear that these are not used here.

(In reply to Martin Stránský from comment #8)
> It's a bit confusing that we need to get MOZ_GTK_PROGRESS_TROUGH instead of
> MOZ_GTK_PROGRESSBAR just because of the style save/restore in
> moz_gtk_progress_chunk_paint().

Yes, but the comment helps, thanks.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch for check-in

Thanks. There's the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad7620700190

Comment 11

[Hussam Al-Tayeb]

The current patch no longer applies to trunk.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch for check-in

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31d6f1e7c98c

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27e93cb03221

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/27e93cb03221