Closed Bug 1273013 Opened 3 years ago Closed 3 years ago

download progress bar does not show in gtk3 build.

Categories

(Core :: Widget: Gtk, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ht990332, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Attached image 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.
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Blocks: gtk3
What is the GTK version and theme?
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.
No need to compile with gtk2, thanks.
This was a deliberate change in GTK 3.20.
Blocks: gtk-3.20
No longer blocks: gtk3
Attached patch wip patch (obsolete) — Splinter Review
Needs Bug 1271579 checked in.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Applying both patches to my local tree makes the progress bar appear again. Thank you Martin.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8752868 - Attachment is obsolete: true
Attachment #8753369 - Flags: review?(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.
Attachment #8753369 - Flags: review?(karlt)
Attached patch patch v.2Splinter Review
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.
Attachment #8753369 - Attachment is obsolete: true
Attachment #8754009 - Flags: review?(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.
Attachment #8754009 - Flags: review?(karlt) → review+
The current patch no longer applies to trunk.
https://hg.mozilla.org/mozilla-central/rev/27e93cb03221
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.