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
Add WebGL escape hatch #5494
Add WebGL escape hatch #5494
Changes from all commits
fc42102
31a5370
4f9dec7
7c57f78
7e647de
6a2f128
07e41d4
0f54bb0
8f8472a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
==============================================================================*/ | ||
import {Injectable} from '@angular/core'; | ||
|
||
const FORCE_SVG_RENDERER_KEY = '_tb_force_svg'; | ||
|
||
@Injectable() | ||
export class ForceSvgDataSource { | ||
constructor() {} | ||
|
||
getForceSvgFlag(): boolean { | ||
if (localStorage.getItem(FORCE_SVG_RENDERER_KEY)) { | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
updateForceSvgFlag(forceSvgFlag: boolean) { | ||
if (forceSvgFlag) { | ||
localStorage.setItem(FORCE_SVG_RENDERER_KEY, 'present'); | ||
} else { | ||
localStorage.removeItem(FORCE_SVG_RENDERER_KEY); | ||
} | ||
} | ||
} | ||
|
||
export const TEST_ONLY = { | ||
FORCE_SVG_RENDERER_KEY, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
==============================================================================*/ | ||
|
||
import {NgModule} from '@angular/core'; | ||
import {ForceSvgDataSource} from './force_svg_data_source'; | ||
|
||
@NgModule({ | ||
providers: [ForceSvgDataSource], | ||
}) | ||
export class ForceSvgDataSourceModule {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of creating its own module I think you could just provide ForceSvgDataSource module in FeatureFlagModule. That eliminates a little bit of the overhead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried that at first. But including that in the effects build creates a circular dependency. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
==============================================================================*/ | ||
import {TestBed} from '@angular/core/testing'; | ||
import {LocalStorageTestingModule} from '../util/local_storage_testing'; | ||
import {ForceSvgDataSource, TEST_ONLY} from './force_svg_data_source'; | ||
|
||
describe('feature_flag/force_svg_util test', () => { | ||
let dataSource: ForceSvgDataSource; | ||
let getItemReturnValue: string | null; | ||
let getItemSpy: jasmine.Spy; | ||
let setItemSpy: jasmine.Spy; | ||
let removeItemSpy: jasmine.Spy; | ||
|
||
beforeEach(async () => { | ||
await TestBed.configureTestingModule({ | ||
imports: [LocalStorageTestingModule], | ||
providers: [ForceSvgDataSource], | ||
}).compileComponents(); | ||
|
||
getItemSpy = spyOn(window.localStorage, 'getItem').and.callFake( | ||
(key: string) => { | ||
return getItemReturnValue; | ||
} | ||
); | ||
setItemSpy = spyOn(window.localStorage, 'setItem').and.stub(); | ||
removeItemSpy = spyOn(window.localStorage, 'removeItem').and.stub(); | ||
dataSource = TestBed.inject(ForceSvgDataSource); | ||
}); | ||
|
||
describe('#getForceSVG', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Newline before this line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. |
||
it('returns false if localStorage.getItem returns null', () => { | ||
getItemReturnValue = null; | ||
const actual = dataSource.getForceSvgFlag(); | ||
expect(getItemSpy).toHaveBeenCalledOnceWith( | ||
TEST_ONLY.FORCE_SVG_RENDERER_KEY | ||
); | ||
expect(actual).toBeFalse(); | ||
}); | ||
|
||
it('returns true if there is a value returned by localstorage.getItem with the key "forceSVG"', () => { | ||
getItemReturnValue = 'this should not matter'; | ||
const actual = dataSource.getForceSvgFlag(); | ||
expect(getItemSpy).toHaveBeenCalledOnceWith( | ||
TEST_ONLY.FORCE_SVG_RENDERER_KEY | ||
); | ||
expect(actual).toBeTruthy(); | ||
}); | ||
}); | ||
|
||
describe('updateForceSVG', () => { | ||
it('creates localStorage entry with key forceSVG when passed truthy', () => { | ||
dataSource.updateForceSvgFlag(true); | ||
expect(setItemSpy).toHaveBeenCalledOnceWith( | ||
TEST_ONLY.FORCE_SVG_RENDERER_KEY, | ||
jasmine.any(String) | ||
); | ||
}); | ||
it('calls localStorage.removeItem with key forceSVG', () => { | ||
dataSource.updateForceSvgFlag(false); | ||
expect(removeItemSpy).toHaveBeenCalledOnceWith( | ||
TEST_ONLY.FORCE_SVG_RENDERER_KEY | ||
); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,4 +50,7 @@ export interface FeatureFlags { | |
// Whether to enable time-namespaced state and how it impacts how user | ||
// settings are kept during navigation. | ||
enabledTimeNamespacedState: boolean; | ||
// Flag for the escape hatch from WebGL. This only effects the TimeSeries | ||
// Scalar cards. | ||
forceSvg: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document? Worth mentioning this is for TimeSeries Scalars cards only (I think?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not using the LocalStorage abstraction that is used by PersistentSettingsDataSource:
https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts;l=198;drc=b0f27571ce3e6ce322b9f1b91a4e8773f9ca47e0
I'm ok with that if you are -- I would lean towards LocalStorage being a bit of an overkill abstraction. But it would be nice to align PersistentSettingsDataSource and ForceSvgDataSource one way or the other. Mind sending a followup PR to remove LocalStorage abstraction use completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to do that. I see it as a useless abstraction. I am not a fan of useless abstractions. The basic localStorage API works great by itself(I might be a bit biased because my last team owned that API :P)