Linux native notifications: Remove spec_version check

Linux native notifications: Remove spec_version check

The spec version check was added to blacklist servers that don't
support icons.  However, some servers like xfce4-notifyd list a spec
version that doesn't have icons when the server actually does support
icons.  More context can be found on:
https://codereview.chromium.org/2883983004/

BUG=676220
R=peter@chromium.org,thestig@chromium.org

Review-Url: https://codereview.chromium.org/2891823002
Cr-Commit-Position: refs/heads/master@{#472953}
Committed: https://chromium.googlesource.com/chromium/src/+/6f8167514ca5a2fc2235cd4694f60598cd49bbf6

[Lei Zhang, 2017-05-17 22:20:42]

So we are just going to look at capabilities and go off of that, since we check
capabilities anyway?

[Tom Anderson, 2017-05-17 23:37:10]

Description was changed from

==========
Linux native notifications: Remove spec_version check

The spec version check was added to blacklist servers that don't
support icons.  However, some servers like xfce4-notifyd list a spec
version that doesn't have icons when the server actually does support
icons.  More context can be found on:
https://codereview.chromium.org/2883983004/

BUG=676220
R=peter@chromium.org,thestig@chromium.org
==========

to

==========
Linux native notifications: Remove spec_version check

The spec version check was added to blacklist servers that don't
support icons.  However, some servers like xfce4-notifyd list a spec
version that doesn't have icons when the server actually does support
icons.  More context can be found on:
https://codereview.chromium.org/2883983004/

BUG=676220
R=peter@chromium.org,thestig@chromium.org
==========

[Tom Anderson, 2017-05-18 01:06:52]

On 2017/05/17 22:20:42, Lei Zhang wrote:
> So we are just going to look at capabilities and go off of that, since we
check
> capabilities anyway?

There's no capability that corresponds to icons being supported.  "icon-static"
and "icon-multi" both refer to the app_icon (I believe).

There are servers (Enlightenment and LXQt notification daemon) that don't have
icon-static but do support images, and there is 1 server (Avant window
navigator) that has icon-static but doesn't support images.

Previously, I thought that all servers supported images since there's no
capability for them.  I guess this is just a limitation of the spec..

[Tom Anderson, 2017-05-18 01:07:22]

On 2017/05/18 01:06:52, Tom Anderson wrote:
> On 2017/05/17 22:20:42, Lei Zhang wrote:
> > So we are just going to look at capabilities and go off of that, since we
> check
> > capabilities anyway?
> 
> There's no capability that corresponds to icons being supported. 
"icon-static"
> and "icon-multi" both refer to the app_icon (I believe).
> 
> There are servers (Enlightenment and LXQt notification daemon) that don't have
> icon-static but do support images, and there is 1 server (Avant window
> navigator) that has icon-static but doesn't support images.
> 
> Previously, I thought that all servers supported images since there's no
> capability for them.  I guess this is just a limitation of the spec..

Oops, closed on accident

[Lei Zhang, 2017-05-18 01:20:22]

OK, so I guess my questions are:

1) What version does Avant window navigator, which doesn't support images,
return?
2) Does anything bad happen if we send image_path and/or image-path to such a
server?
3) Does anything bad happen if we send image_path and image-path to a server
that supports images?

[Tom Anderson, 2017-05-18 01:27:35]

On 2017/05/18 01:20:22, Lei Zhang wrote:
> OK, so I guess my questions are:
> 
> 1) What version does Avant window navigator, which doesn't support images,
> return?
1.0
> 2) Does anything bad happen if we send image_path and/or image-path to such a
> server?
No
> 3) Does anything bad happen if we send image_path and image-path to a server
> that supports images?
there will be an image.  That's not bad, right :)

[Lei Zhang, 2017-05-18 02:25:02]

On 2017/05/18 01:27:35, Tom Anderson wrote:
> On 2017/05/18 01:20:22, Lei Zhang wrote:
> > OK, so I guess my questions are:
> > 
> > 1) What version does Avant window navigator, which doesn't support images,
> > return?
> 1.0
> > 2) Does anything bad happen if we send image_path and/or image-path to such
a
> > server?
> No
> > 3) Does anything bad happen if we send image_path and image-path to a server
> > that supports images?
> there will be an image.  That's not bad, right :)

Sounds like you think most servers are tolerant and won't have any troubles with
this. I guess we can try this and see if we get any bug reports from users.

The alternative is to be strict and just leave it as is. In which case, only
XFCE users will be missing out on native notifications. They can wait for an
eventual fix from upstream, or fix it locally with a rebuilt binary, or just not
have native notifications.

Let's see if Peter has any strong feelings about this. I don't either way.

[Peter Beverloo, 2017-05-18 20:12:17]

On 2017/05/18 02:25:02, Lei Zhang wrote:
> On 2017/05/18 01:27:35, Tom Anderson wrote:
> > On 2017/05/18 01:20:22, Lei Zhang wrote:
> > > OK, so I guess my questions are:
> > > 
> > > 1) What version does Avant window navigator, which doesn't support images,
> > > return?
> > 1.0
> > > 2) Does anything bad happen if we send image_path and/or image-path to
such
> a
> > > server?
> > No
> > > 3) Does anything bad happen if we send image_path and image-path to a
server
> > > that supports images?
> > there will be an image.  That's not bad, right :)
> 
> Sounds like you think most servers are tolerant and won't have any troubles
with
> this. I guess we can try this and see if we get any bug reports from users.
> 
> The alternative is to be strict and just leave it as is. In which case, only
> XFCE users will be missing out on native notifications. They can wait for an
> eventual fix from upstream, or fix it locally with a rebuilt binary, or just
not
> have native notifications.
> 
> Let's see if Peter has any strong feelings about this. I don't either way.

I'm fine either way. I have a preference for the servers to just fix their
implementations, given that XFCE is just returning the wrong value, but you
(Tom) already explained why that isn't as straightforward as I had hoped for.

Linux users are much more likely to report issues when they run in to them, so I
have no doubts that issues will find us.

lgtm

[Tom Anderson, 2017-05-18 20:14:13]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 20:15:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 22:17:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 22:17:29]

Dry run: This issue passed the CQ dry run.

[Tom Anderson, 2017-05-18 22:24:53]

Looks like Xfce fixed the version just now
https://git.xfce.org/apps/xfce4-notifyd/commit?id=0273941351c9aad1fd76a6fdaab...

However, the fix will take a long time to reach all distros, so I'm going to
land this anyway.  We can revert it in several months (or years).

[Tom Anderson, 2017-05-18 22:24:59]

The CQ bit was checked by thomasanderson@chromium.org

[commit-bot: I haz the power, 2017-05-18 22:27:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 22:36:55]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495146299436460,
"parent_rev": "9d72fe409a214856ec4d9700105b4664bb4c0861", "commit_rev":
"6f8167514ca5a2fc2235cd4694f60598cd49bbf6"}

[commit-bot: I haz the power, 2017-05-18 22:37:14]

Description was changed from

==========
Linux native notifications: Remove spec_version check

The spec version check was added to blacklist servers that don't
support icons.  However, some servers like xfce4-notifyd list a spec
version that doesn't have icons when the server actually does support
icons.  More context can be found on:
https://codereview.chromium.org/2883983004/

BUG=676220
R=peter@chromium.org,thestig@chromium.org
==========

to

==========
Linux native notifications: Remove spec_version check

The spec version check was added to blacklist servers that don't
support icons.  However, some servers like xfce4-notifyd list a spec
version that doesn't have icons when the server actually does support
icons.  More context can be found on:
https://codereview.chromium.org/2883983004/

BUG=676220
R=peter@chromium.org,thestig@chromium.org

Review-Url: https://codereview.chromium.org/2891823002
Cr-Commit-Position: refs/heads/master@{#472953}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f8167514ca5a2fc2235cd4694f6...
==========

[commit-bot: I haz the power, 2017-05-18 22:37:19]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6f8167514ca5a2fc2235cd4694f6...