Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed vertical slider check when obfuscated #1303

Closed
wants to merge 2 commits into from
Closed

Fixed vertical slider check when obfuscated #1303

wants to merge 2 commits into from

Conversation

TheUltDev
Copy link
Contributor

In #477, support for vertical slider computation was added.
The config check breaks when obfuscated, it ends up being something like if(a.j.Wd)

I've just started to familiarize myself with the code-base, but I believe this.options().vertical is the correct way to check an option. This should solve the obfuscation issue.

In #477, support for vertical slider computation was added.
The config check breaks when obfuscated, it ends up being something like `if(a.j.Wd)`

I've just started to familiarize myself with the code-base, but I believe `this.options().vertical` is the correct way to check an option. This should solve the obfuscation issue.
@gkatsev
Copy link
Member

gkatsev commented Jun 19, 2014

this.options() and this.options_ are equivalent within vjs itself. The correct fix is to actually make vertical a string.
Though, this seems to have been working alright in the players I've been working with.

@TheUltDev
Copy link
Contributor Author

The difference being vjs.Component.prototype.options is listed in the externs.

I see there are some instances in the code where code like this.options()['example'] are used.
Likewise, this.options().example is used very often as well. Without the above mentioned extern, the latter instances would break.

Ignoring the fact that it's breaking on obfuscation, this.options_ should not have been used outside of the Components class in the first place, it's supposed to be a private property.

Edit: changed the property to a string. Kept the options() call instead of accessing a private property.

@gkatsev
Copy link
Member

gkatsev commented Jun 19, 2014

Well, it's supposed to be a internal to videojs, so, any videojs code can use it. Just don't use it outside (also, because it wont exist outside, you can't use it anyway).

But I guess this is fine too, since they are equivalent.

Also, thanks for the PR!

@heff
Copy link
Member

heff commented Jun 23, 2014

lgtm!

@sethborg
Copy link
Contributor

There are a lot of places in the code where you could save 3 bytes by switching from array notation to dot notation. Here's one place where the two are intermixed. Although, apparently the minifying code fixes this anyway.

LINE 6146: vjs.TEST_VID['volume'] = 0.5;
LINE 6168: var volume = vjs.TEST_VID.volume;

@heff
Copy link
Member

heff commented Jun 23, 2014

The array notation is necessary in some places where vars need to be external. Otherwise closure compiler aggressively mangles them. The volume example sneaks by because closure compiler has a default set of 'externs', the protect html-defined properties (including video element properties like volume) from getting mangled.

@heff heff closed this in b66ef80 Jun 26, 2014
@heff
Copy link
Member

heff commented Jun 26, 2014

Thank you!

@TheUltDev TheUltDev deleted the patch-1 branch June 26, 2014 17:05
@TheUltDev
Copy link
Contributor Author

No problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants