Hussam Al-Tayeb Reporter
	Description • 8 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 1 • 8 years ago

What is the GTK version and theme?

	Hussam Al-Tayeb Reporter
	Comment 2 • 8 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 3 • 8 years ago

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

	Martin Stránský [:stransky] (ni? me)
	Comment 4 • 8 years ago

Needs Bug 1271579 checked in.

	Hussam Al-Tayeb Reporter
	Comment 5 • 8 years ago

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

	Martin Stránský [:stransky] (ni? me)
	Comment 6 • 8 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 7 • 8 years ago

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.

	Martin Stránský [:stransky] (ni? me)
	Comment 8 • 8 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 9 • 8 years ago

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.

	Martin Stránský [:stransky] (ni? me)
	Comment 10 • 8 years ago

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

	Hussam Al-Tayeb Reporter
	Comment 11 • 8 years ago

The current patch no longer applies to trunk.

	Martin Stránský [:stransky] (ni? me)
	Comment 12 • 8 years ago

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

	Pulsebot
	Comment 13 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 8 years ago
bugherder

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