1275160 - Web Manifest: don't special case orientation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcos Caceres [:marcosc]]

The spec recently changed to no longer require orientation to return an empty string. The manifest processor + tests needs to be updated to match.

Comment 1

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1378e7cccf35

Comment 2

[Marcos Caceres [:marcosc]]

Attachment: tiny change to how orientation member is processed

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8755699 [details] [diff] [review]
tiny change to how orientation member is processed

Review of attachment 8755699 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/manifest/ManifestProcessor.jsm
@@ +149,3 @@
>        if (value && typeof value === "string" && this.orientationTypes.has(value.toLowerCase())) {
>          return value.toLowerCase();
>        }

This might sound pedantic, but I think I'd prefer if we explicitly return undefined here if that's expected. I suspect that's something a linter would pick up too.

Comment 4

[Marcos Caceres [:marcosc]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #3)
> This might sound pedantic, but I think I'd prefer if we explicitly return
> undefined here if that's expected.

No, that's totally reasonable. I should have kept it in from the original. 

> I suspect that's something a linter would
> pick up too.

Heh, I think something like JSHint would complain for the opposite reason: returning undefined is redundant :) However, I also agree that it's clearer. 

Thanks for the speedy review!

Comment 5

[Marcos Caceres [:marcosc]]

Attachment: Explicitly return undefined.

Carrying over r+

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8749c914b0

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b8749c914b0